WIP: Review usage of mutables
In a multi-threaded world, immutability is a very valuable property, because it provides a statically checked protection against data races. This property is in fact so important that C++11 famously changed the meaning of a const method to mean "safe to call from multiple threads", and uses this definition in the STL in order to determine which object methods are thread-safe.
This means that the keyword "mutable" should be used even more sparingly than before. It is basically only suitable for carefully designed thread-safe caches (think linux RCU), and mutexes or similar synchronization constructs that must be modified to read the state of an object.
Bearing this in mind, I have set out, as part of ACTS-256, to go through the ACTS codebase and eliminate every use of mutable which does not fit the C++11 definition of "const".
This work is not finished, but I'm submitting it now for the benefit of early review. Here's what I've done so far:
- Remove all mutable class members which should not be mutable according to the previous definition (i.e. all of them)
- Made corresponding class setters non-const
- Replaced const-ref parameters with non-const-ref parameters in methods that modify their parameters
- Used const_cast and const_pointer_cast to mutate pointer-to-const members during object construction
- Replaced several instances of lazy initialization on readout with eager initialization.
- Made object factories return non-const pointers (turns out one frequently wants to mutate a factory-provided object before returning it)
- Added non-const accessors to members of non-const objects when by-reference member access is used.
- Introduced a pointer wrapper that helps making classes with pointer members const-correct (see below)
I'm not too happy about all the const_casts that I've introduced. It's better than the previous situation, because objects are now truly immutable after construction, but const-casting is definitely an error-prone practice that I would like to do away with. So I want to get rid of these casts next. And the way I intend to do so is essentially by simplifying C++'s complex pointer constness semantics into something simpler, yet powerful enough for our needs.
In C++, pointers can have many constness states:
-
T*= "can modify pointer and pointed object" -
const T*= "can modify pointer, but not pointed object" -
T* const= "can modify pointed object, but not pointer" -
const T* const= "can modify neither pointer nor pointer object"
Now, this is all fine and good and flexible, but from a const-correctness point of view, there are really only two pointer constness state that matter: T* and const T* const. And in C++, getting from one of these states to the other requires a const_cast.
The way I intend to fix this is by introducing a lightweight pointer wrapper which behaves like a T* if non-const and like a const T* const if const. I'll do so both for pointers and smart pointers, and use that in place of raw pointers for classes with problematic pointer members.
This wrapper will be fully inlined, and to the best of my understanding should have zero run-time space and CPU overhead. It will only change the way the compiler behaves when the pointer wrapper is made const, into something more intuitive and respectful of const correctness.
A prototype of this wrapper, called ConstCorrectPtr, is now present in the merge request.
This merge request closes ACTS-256, or at least will do so once ready and accepted.