When you have a class like
class C {
C(): hasId(false) {}
// ...
private:
// ...
bool hasId;
int id; // iff hasId
};
instances of which may under certain circumstances be labeled with an ominous, externally inflicted ID (with legal values anywhere in the INT_MIN–INT_MAX range), Coverity Scan will tell you that you have a medium-sized security hole and you should better initialize that id scalar member variable in the C constructor.
Is that helpful? I doubt it.
What shall I initialize it with? Zero? Why zero, what’s so special about zero? If the invariant for C was that “id contains a valid ID iff hasId is true” before, has that now changed to the more complex “if hasId is true, then id contains a valid ID, otherwise id contains zero?” If I do maintenance work on that class a year from now, do I need to worry to reset id to zero when I reset hasId to false in some place in the code?
A better value to initialize such a member variable with would arguably be bottom. After all, every C++ type is inhabited by that. But, alas, C++ doesn’t give you first-class access to bottom as a value to asign to a variable. The closest approximation is probably to leave the variable uninitialized and allow tools like Valgrind to detect violations of the invariant at runtime. The exact thing that Coverity Scan advises you not to do. Oh my.
So a programming error that would have been identified by Valgrind becomes a programming error that is drowned in a too-complex class invariant? “Defensive programming” deluxe?
Another option would be to combine hasId and id into a single member variable of type boost::optional<int>, but that can become unwieldy if multiple members shall be controlled by one conditional bool. Keep it simple.
A somewhat acceptable approach to make Coverity Scan happy might be to explicitly value-initialize id via an empty set of parentheses,
C(): hasId(false), id() {}
That would still initialize id to that funny zero, but would at least give the adept reader a clue that it shall have no influence on any invariant, and is merely there to appease some automatic analyzer.
Forgive my rant, it’s Friday.
“leave the variable uninitialized and allow tools like Valgrind to detect violations of the invariant at runtime.”
That only works for the times you actually run the program under Valgrind. Other users of your (buggy) program will just use the unpredictable uninitialised value and end up doing something unexpected, possibly damaging their data. If all possible ids really are legal, then boost::optional<int> does sound like a better option, because it actaully encapsulates the very thing you’re trying to express. If you have multiple members which may be optional together, make a private struct in your class to contain them, and use boost::optional<members> to express that.
When you come to read the code again in six months time to fix a bug/add a feature, future you will thank your past self for writing code that explicitly describes exactly what it is doing. You won’t need a comment saying…
bool hasMembers; // Covers members .foo, .bar and .baz. Do not access them if false.
…which, when you later add .quux that is also covered by it, you’ll totally forget to update, causing much hilarity^Wwailing and gnashing of teeth when another maintenance programmer does something else with quux, not realising they need to check hasMembers first…
One variable telling you if another variable is valid? If at all possible, don’t do that.
Just discovered that Rusty Russell had blogged about this topic before, “The Power of Undefined Values”.
optional is clearly the right solution here. And if you want more than one member to be controlled by the same flag, use a tuple or a struct in the optional. It’s not hard and certainly not an excuse to employ unsafe coding practises.