Attachment 'const_discussion.txt'
Download 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++.
Attached Files
To refer to attachments on a page, use attachment:filename, as shown below in the list of files. Do NOT use the URL of the [get] link, since this is subject to change and can break easily.You are not allowed to attach a file to this page.