1 Malte Helmert
   2 I see, I did not consider the implicit operator=, which is of course a non-const
   3 method. For what it's worth, I dislike assignment for classes like TaskProxy,
   4 precisely because we like our objects to be immutable where possible, and
   5 consequently we should use assignment very sparingly. For example, we never use
   6 TaskProxy::operator= in the default branch of the master repository (I just
   7 tested this by setting the method to delete), and I think this is as it should
   8 be. More generally, I think that a large number of our classes (if I'd guess,
   9 I'd say about half?) are logically immutable, and for these I think a case could
  10 be made for deleting operator= to actually make them immutable. There may be a
  11 few cases where we actually use assignment, but for me it’s a bit of a code
  12 smell for classes like these because it violates immutability and thus makes
  13 passing references dangerous. The problem I have with this const is that if we
  14 use it here, there are literally hundreds of other places in the code where we
  15 should logically use it, too. I just don’t think this place is any different
  16 from any of the other related places. Do I remember correctly that there were
  17 recently more discussions about this (including Silvan?). I think if we use
  18 const here, then we should use it in all other places where it makes sense, too.
  19 I may remember wrongly, but I think we considered that at some point and found
  20 it too cumbersome mainly because of the inconvenience it would cause in
  21 constructors. That may no longer apply, I’m not sure. I wouldn’t mind
  22 reconsidering that question, but I think that would require a targeted
  23 discussion of the topic also including the other developers. Here is the issue I
  24 have with using const inconsistently: if we consistently don’t use const for
  25 attributes (except for const pointers or references, where the semantics are
  26 different), which I think is currently how the large majority of our code works,
  27 then we don't get a signal about constness just from looking at the data members
  28 of a class. This isn't great, of course, but at least it means that we know we
  29 need to understand the class better to decide which of the members change and
  30 which don’t. I would say that for most classes this is very obvious. In
  31 comparison, if we use const but use it inconsistently, then we frequently get a
  32 wrong signal (seeing something is non-`const` although it is actually logically
  33 const, we would draw a wrong conclusion), and I think that’s much worse than no
  34 signal, not because it’s “dangerous” but because it increases cognitive load. So
  35 I think this is something we should do consistently or not at all, and I think
  36 right now the balance of the code is “not at all”. I can see the argument in
  37 favour of “do it consistently”, but I think this requires an active decision and
  38 change of policy. So, in a nutshell, I think an inconsistently used const is
  39 worse than no const. And more importantly, "no constis currently the prevailing
  40 convention in the codebase, and it causes a lot of friction if people pull in
  41 different directions regarding such coding conventions. I had a look at the
  42 header files in the default branch to see in how many classes we have a
  43 TaskProxy member, and in how many we have a const TaskProxy member. I found 10
  44 of the former and only 2 of the latter (both in the cegar code). All of these
  45 could be const because we never assign to TaskProxy. (The code still compiles
  46 when deleting assignment.) I see on our coding conventions page in the wiki that
  47 it mentions that TaskProxy attributes may be const. (Now prizes for guessing who
  48 added that bit. 😉) I suggest we change that for now, removing the “(const)”
  49 bit. As I said, I’m open to a change of policy, but that has to be an explicit
  50 discussion also including the others. I think this is the third time we’re
  51 having the same discussion (with various configurations of people) in a month,
  52 so I think it is costing us a lot of effort.
  53 
  54 SilvanS
  55 Yes, I once tried to convince Jendrik that I diddn’t want const bool for some
  56 class attributes that I was introducing. so I'm a bit surprised to see that we
  57 (again?) go away from this, because I started trying to add const at many places
  58 where I would not have added them if I wasn’t told to do so 🙂 Anyway, glad that
  59 I don’t have to bother with this anymore and just follow my previous intuittion
  60 that const only makes sense with pointers and references, basically.
  61 
  62 Malte Helmert
  63 I wouldn’t say that const only makes sense with pointers and references. What
  64 Jendrik wrote about having certainty that these attributes will never change is
  65 a good argument in favour of const attributes. If this were a new codebase, I
  66 would certainly give a more rigorous use of const a try, and if someone wants to
  67 drum up support for a change in coding conventions here, we can discuss it. It's
  68 just the current convention that says not to use const, not something I’d
  69 consider a general stylistic recommendation for C++.