const and mutable are powerful tools for writing safer code. Use them consistently.
Problem
Guru Question
In the following code, add or remove const (including minor variants and related keywords) wherever appropriate. Note: Don’t comment on or change the structure of this program. It’s contrived and condensed for illustration only.
For bonus points: In what places are the program’s results undefined or uncompilable due to const errors?
class polygon {
public:
polygon() : area{-1} {}
void add_point( const point pt ) { area = -1;
points.push_back(pt); }
point get_point( const int i ) { return points[i]; }
int get_num_points() { return points.size(); }
double get_area() {
if( area < 0 ) // if not yet calculated and cached
calc_area(); // calculate now
return area;
}
private:
void calc_area() {
area = 0;
vector<point>::iterator i;
for( i = begin(points); i != end(points); ++i )
area += /* some work using *i */;
}
vector<point> points;
double area;
};
polygon operator+( polygon& lhs, polygon& rhs ) {
auto ret = lhs;
auto last = rhs.get_num_points();
for( auto i = 0; i < last; ++i ) // concatenate
ret.add_point( rhs.get_point(i) );
return ret;
}
void f( const polygon& poly ) {
const_cast<polygon&>(poly).add_point( {0,0} );
}
void g( polygon& const poly ) { poly.add_point( {1,1} ); }
void h( polygon* const poly ) { poly->add_point( {2,2} ); }
int main() {
polygon poly;
const polygon cpoly;
f(poly);
f(cpoly);
g(poly);
h(&poly);
}
Sutter’s Mill 
As mentioned in GotW#4 Class Mechanics solution, operator + can be changed to:
// add const& to rhs, remove const& from lhs polygon operator+( polygon lhs, const polygon& rhs ) { auto last = rhs.get_num_points(); for( auto i = 0; i < last; ++i ) // concatenate lhs.add_point( rhs.get_point(i) ); return lhs; }to avoid unnecessary copy auto ret = lhs;, if you’re going to do the copy anyway.
polygon() : area{-1} {}I would rather initialise area with 0 or, even better, completely remove the area member from the class since its usefulness is discussable. Constructors may be left to be synthesised by the compiler (and they probably will be synthesised due to non-trivial constructors of the points member). This will save sizeof(double) of memory per object as well.
void add_point( const point pt ) { area = -1; points.push_back(pt); }Function argument is passed by copy anyway, moreover it may be copied to the vector once again. I think that constness of pt will not affect the push_back behaviour. I would suggest to pass const point& pt or use pt’s move constructor to “move” the pt’s memory to the vector (please correct me if it is not really possible).
point get_point( const int i ) { return points[i]; }The value of the i may be out of bounds, this should be checked first. Const on fundamental type does nothing I suppose, it may be removed freely. Getter in general should be non-modifying, so with const modifier on a member function. Having getter in const context the points member needs to be mutable and guarded with a mutex. I wonder what is really returned from this member function, is this a “moved” item from the points vector (destructive read?) or a real copy of the points[i] item. I would try to return a const& rather than an object, but it may be pre-C++11 way of doing things…
int get_num_points() { return points.size(); }I would suggest to change return type to size_t, make above getter const modified and protect the points vector with a mutex (c.f. my comment on get_point and mutable).
double get_area() {I wonder how caching of a value calculated at some time is valid when multiple threads operates on the same object. I think it is not a good idea to do caching. It may break function “re-entrancy” since now such function does not depend on the actual state of the points vector solely.
By the way, public member function calls private member function – this recalls me the rule to make virtuals private and call them through non-virtual public interface, unfortunately calc_area is not a virtual function.
void calc_area() {This member function has read-only access to the points vector, so it should be const modified and the points member access should be protected with a mutex. Anyway, I would use std::accumulate (or custom functor) with const iterators and return the calculated value to the caller.
polygon operator+( polygon& lhs, polygon& rhs ) { auto ret = lhs;I suppose we don’t want to modify lhs, so let’s make it const&. It may be quite good to add a check that will prevent from adding to itself. I would define operator+= as well. I wonder what is really stored under the auto type for ret. Is this simply a polygon type (without &)?
const_cast<polygon&>(poly).add_point( {0,0} );This code will not pass a code review round :-)
void g( polygon& const poly ) { poly.add_point( {1,1} ); }I think that &const is redundant here since reference by definition is const “pointer” already.
The g() looks like a better way to write h().
muxecoid: ret is a copy of, not a reference to, lhs. When you’re using auto you still need to specify a reference qualifier (&) if that’s what you want
At first glance:
Apparently operator+ modifies lhs which is not good. Adding const to lhs in function definition should cause it to fail to compile.
area should be mutable. calc_area can become const (and gets a lock at start of function body).
The overload of f that accepts const reference should not exist.
get_point obviously needs const.
In h it is the value of the pointer which is const, not the actual poly.
– change add_point() signature to add_point(const point &pt)
– change get_point(), get_num_points() and get_area() to be const members functions
– change calc_area() to const member function and change area member variable to be mutable since it’s cached value
– in calc_area() change i to be const_iterator
– change parameters to operator+() to be passed by const reference
– change polygon to be passed to f() by non-const reference and remove const_cast from impl. since passing by const violates the contract of non-mutable argument
– change g(polygon& const poly) to g(polygon&)
– change polygon to h() be passed by non-const reference since impl. expect non-null pointer
– f(cpoly); behavior is undefined since cpoly may be allocated from immutable protected const store that can’t be modified.
additional notes:
– depending on the expected usage pattern, it might be good idea to calculate area in add_point() function instead of using the lazy caching mechanism. this would make access to get_area() faster since there is no if() branching required and then area variable wouldn’t need to be mutable (it’s only changed in non-const add_point() function). e.g. if add_point() for a polygon is expected to be called only ~10 times while get_area() is expected to be called ~100000 times, it’s better not to use the lazy area caching mechanism. Furthermore, it would be good idea in this case to add add_points() which takes multiple points in one go and updates the area only once to enable client side optimizations.
– calc_area() could just return the calculated area and not update the area member variable. the update of the variable could be done in get_area() instead. this keeps calc_area() truly const and limits the mutability of the object only to get_area() function.
add_point() should take a point by nonconst value, and move it into the vector.
get_point() should be const. Its parameter needn’t be const.
get_num_points() should be const and should return size_t.
get_area() should be const. area should be mutable.
calc_area() should be const and should use const iterators into the vector.
operator+ should take lhs by nonconst value (and then not copy it again into ‘ret’), and rhs by reference-to-const.
f should take its argument by reference-to-nonconst, since it modifies it. Of course there’s no need for the const_cast then.
g has a const in a place where it’s implied (and not allowed explicitly) – after a reference. That should be removed. Otherwise it’s fine.
h is fine, the toplevel const just means h promises not to change its argument poly to point to a different object. However, as with get_point(), there is little point (no pun intended) to making a parameter toplevel const.
Buffering “area” looks like premature optimization. One could “fix” the code, make it mutable, add a mutable mutex, but – without further demand – the cleaner solution would be to get rid of it and offer a non-member “double calc_area(const polygon&)” function to keep the polygon interface “minimal and complete”. Interface design would be improved and thread-safety comes as an additional “present”.
This is definitely uncompilable (always, regardless of the function’s body), since there’s no such thing as a const-reference (only a reference-to-const), as the reference-referee binding is always invariant:
This would also be uncompilable, but this time solely due to the actual body of the function “g” being in violation of the const-contract (which we’ve promised by using the reference-to-const):
void g( polygon const & poly ) { poly.add_point( {1,1} ); }Hence, to fix the uncompilable aspect (not commenting on the other aspects here), we can start with:
void g( polygon & poly ) { poly.add_point( {1,1} ); }