GotW #6b Solution: Const-Correctness, Part 2

C++ 9 Minutes

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);
}

 

Solution

When I pose this kind of problem, I find that most people think the problem is on the easy side and address only the more-usual const issues. There are, however, subtleties that are worth knowing, and hence this Item.

1. The point object is passed by value, so there is little benefit to declaring it const.

    void  add_point( const point pt )

In this particular case, because the function is defined inline, the const value parameter can make sense. This is because for inline functions the declaration and definition are the same. Otherwise, const value parameters should appear only on the definition, not on the declaration. Let’s see why.

Putting the const on a value parameter in a function declaration is irrelevant outside the function—it makes no difference to the caller and can only confuse readers. To the compiler, the function signature is the same whether you include this const in front of a value parameter or not. For example: 

// value parameter: top-level const is not part of function signature
int f( int );
int f( const int );    // redeclares f(int): this is the same function

// non-value parameter: top-level const is part of function signature
int g( int& );
int g( const int& );   // overloads g(int&): these are two functions

But putting the const on the value parameter does make a difference to how it can be used inside the function’s actual definition. Remember that, inside a function, the parameters are just the first set of local variables, so putting a const on a value parameter simply means that the function can’t modify its local variable, which only happens to be a parameter. Here’s an example that declares and then defines the same function f: 

int f( int );          // declaration: no const

int f( const int i ) { // definition: use const to express "read-only"

    vector<int> v;
    v.push_back(i);    // ok, only reads from i

    i = 42;            // error, attempts to modify i

}

Guideline: Consider not writing const on pass-by-value function parameters when only forward-declaring a function. You can always add it on the definition to express a read-only parameter.

2. get_point and get_num_points should be const.

    point get_point( const int i ) { return points[i]; }

    int   get_num_points() { return points.size(); }

These functions should be marked const, because they don’t change the state of the object.

3. get_area should be const.

    double get_area() {
        if( area < 0 )   // if not yet calculated and cached
            calc_area();     // calculate now
        return area;
    }

Even though this function modifies the object’s internal state, we should consider making it const. Why? Because this function does not modify the object’s observable state; we are doing some caching here, but that’s an internal implementation detail and the object is logically const even if it isn’t physically const.

4. Therefore calc_area should also be const.

    void calc_area() {
        area = 0;
        vector<point>::iterator i;
        for( i = begin(points); i != end(points); ++i )
            area += /* some work using *i */;
    }

Once we make get_area be const, this private helper function ought also to be const.

In turn, once you make this function const, the compiler will tell you that you also need to do something about the member variable area, which should be:

  • declared mutable, so that it’s writable in a const member function; and
  • synchronized using a mutex or made atomic<>, so that it’s concurrency-safe, as discussed in GotW #6a.

5. Also, calc_area should use a const_iterator.

The iterator should not change the state of the points collection, and so it ought to be a const_iterator. We’re now forced to make this change anyway if we’re making calc_area be a const member function, but note that if we had said auto for the iterator’s type we wouldn’t have had to make any change at all. While we’re at it, the for loop inside calc_area: It should prefer to use the range-based for loop, as well as auto.

Combining all that, we get this simpler and const-correct code: 

      for( auto& pt : points )
          area += /* some work using pt */;

Guideline: Prefer declaring variables using auto.

Guideline: Prefer range-based for loops to naked iterator-incrementing for loops when visiting the elements of the range in order.

6. area should be mutable and synchronized.

    double        area;

As noted already, in conjunction with the other changes the internal cache variable will area now want to be mutable so that it can be used correctly and safely inside const member functions, and because it is now a shared variable potentially used by multiple concurrent const operations it must also be synchronized—protected with a mutex or made atomic.

Bonus Question: Before reading on, which should it be: Protected by a mutex? or made atomic<double>?

Have you thought about it? All right, let’s continue…

Both work, but a mutex is usually overkill for a single variable.

Option 1 is to use a mutex in the perhaps-soon-to-be-canonical “mutable mutex mutables” pattern: 

// Option 1: Use a mutex 

    double get_area() const {
        auto lock = unique_lock<mutex>{mutables};
        if( area < 0 )   // if not yet calculated and cached
            calc_area();     // calculate now
        return area;
    }

private:
    // ...
    mutable mutex  mutables;      // canonical pattern: mutex that
    mutable double area;          //   covers all mutable members

Option 1 generalizes well if you add more data members in the future. However, it’s also more invasive and generalizes less well if you add more const member functions in the future that use area, because they will all have to remember to acquire a lock on the mutex before using area.

Option 2 is to just change double to mutable atomic<double>. This is attractive because the “mutable part” of polygon is just a single variable. That can work, but you have to be careful because that’s not the only necessary change, for two reasons:

  • The minor reason is that atomic<double> doesn’t support +=, so if we only change area‘s type then calc_area will no longer compile. That can be worked around, but leads us to the major reason…
  • The major reason is that, because calc_area is a compound operation and must be safe to run on multiple threads concurrently, we must restructure calc_area to be safe to run concurrently. In particular it should not perform intermediate updates to area, and should ensure that multiple competing concurrent updates to area don’t cause overwrites that lose written values.

There are several ways to do it, but the simplest is probably to allow benign redundant recalculations in the case of concurrent calls to calc_area, on the grounds that it’s probably no worse than blocking the concurrent calls which would have to wait anyway. 

// Option 2: Use an atomic

    void calc_area() const {
        auto tmp = 0.0;              // do all the work off to the side
        for( auto& pt : points )
            tmp += /* some work using pt */;
        area = tmp;                  // then commit with a single write
    }

private:
    // ...
    mutable atomic<double> area;

Notice that concurrent const operations that call to calc_area can still overlap and overwrite each other’s results, but that’s benign because they’re concurrent const operations so they will all calculate the same value. Also, concurrent calc_area calls use the shared points variable in a loop which should make us mentally check that it doesn’t cause cache contention, but because they’re all readers it won’t and so this too is fine.

7. operator+’s rhs parameter should be a reference to const.

polygon operator+( polygon& lhs, polygon& rhs ) {

The rhs parameter should be passed by reference to const, of course.

Guideline: Prefer passing a read-only parameter by const& if you are only going to read from it (not make a copy of it).

“But wait!” I can just hear some of you saying, “you forgot about lhs! Shouldn’t it be const& too?” Actually, not so much:

8. operator+’s lhs parameter should be passed by value.

The key point is that we’re going to copy from it anyway, in this case immediately: 

    auto ret = lhs;

When you’re in the special case of “read-only parameter that you’re going to take copy of anyway,” there are several ways to accept the parameter, which I’ll cover in detail in another GotW. For now, suffice it to say that usually you shouldn’t overthink these options, and just use pass-by-value as the simplest method, which offers some advantages that we also touched on in GotW #4:

  • If the caller passes a named polygon object (an lvalue), there’s no difference. Both pass-by-const& followed by an explicit copy and pass-by-value will perform one copy.
  • If the caller passes a temporary polygon object (an rvalue), the compiler automatically move-constructs lhs from that, which probably makes no difference for a small type like polygon but can be considerably cheaper for many types.

    Guideline: Prefer passing a read-only parameter by value if you’re going to make a copy of the parameter anyway, because it enables move from rvalue arguments.

9. Also in operator+, last should be const.

    auto last = rhs.get_num_points();
    for( auto i = 0; i < last; ++i ) // concatenate
        ret.add_point( rhs.get_point(i) );
    return ret;
}

Since last should never change, prefer to say so by making it const.

Guideline: Prefer to make variables, including locals, const if they should not change.

Incidentally, notice that once we make rhs a reference-to-const parameter as noted above, we see another reason why get_point should be a const member function.

10. f’s const_cast may give undefined behavior, and is morally wrong anyway.

void f( const polygon& poly ) {
    const_cast<polygon&>(poly).add_point( {0,0} );
}

Bonus: The result of the const_cast is undefined if the referenced object was declared as const—which it is in the case of f(cpoly) below.

The parameter isn’t really const, so don’t declare it as const and then try to modify it anyway. Lying to the compiler, never mind to the caller, is a bad idea, never mind morally reprehensible in most value systems.

11. g’s const is illegal and useless.

void g( polygon& const poly ) { poly.add_point( {1,1} ); }

This const is illegal; you can’t apply const directly to the reference itself, besides which references are already const inasmuch as they cannot be reseated to refer to a different object. 

void h( polygon* const poly ) { poly->add_point( {2,2} ); }

Note that h‘s const merely ensures that h‘s body won’t modify the pointer value. This is the same as the const value parameters in add_point and get_point, and perfectly fine on the definition.

12. Examining the mainline.

int main() {
    polygon poly;
    const polygon cpoly;

    f(poly);

This is fine. 

    f(cpoly);

As already noted, this causes undefined results when f tries to cast away the const-ness of its parameter and then modify it. 

    g(poly);

This is fine. 

    h(&poly);
}

This is fine.

Summary

Here is a revised version of the code that corrects the const issues noted above, but does not attempt to correct any other poor style. Note that because of the atomic member, which is not copyable, we now provide polygon‘s copy and move operations explicitly. 

class polygon {
public:
    polygon() : area{-1} {}

    polygon( const polygon& other ) : points{other.points}, area{-1} { }

    polygon( polygon&& other ) 
        : points{move(other.points)}, area{other.area.load()}
        { other.area = -1; }

    polygon& operator=( const polygon& other )
        { points = other.points; area = -1; return *this; }

    polygon& operator=( polygon&& other ) {
        points = move(other.points);
        area = other.area.load();
        other.area = -1;
        return *this;
    }

    void add_point( point pt ) 
        { area = -1; points.push_back(pt); }

    point get_point( int i ) const { return points[i]; }

    int get_num_points() const { return points.size(); }

    double get_area() const {
        if( area < 0 )   // if not yet calculated and cached
            calc_area();     // calculate now
        return area;
    }

private:
    void calc_area() const {
        auto tmp = 0.0;
        for( auto& pt : points )
            tmp += /* some work using pt */;
        area = tmp;
    }

    vector<point>          points;
    mutable atomic<double> area;
};

polygon operator+( polygon lhs, const polygon& rhs ) {
    const auto last = rhs.get_num_points();
    for( auto i = 0; i < last; ++i ) // concatenate
        lhs.add_point( rhs.get_point(i) );
    return lhs;
}

void f( polygon& poly ) { poly.add_point( {0,0} ); }

void g( polygon& poly ) { poly.add_point( {1,1} ); }

void h( polygon* poly ) { poly->add_point( {2,2} ); }

int main() {
    auto poly = polygon{};

    f(poly);
    g(poly);
    h(&poly);
}

Acknowledgments

Thanks in particular to the following for their feedback to improve this article: mttpd, Vincent Lascaux, jlehrer, Motti, Fernando Pelliccioni, Leo, Mathias Stearn.

Published by Herb Sutter

Herb Sutter is an author and speaker, a software architect at Microsoft, and chair of the ISO C++ standards committee.

Published

54 thoughts on “GotW #6b Solution: Const-Correctness, Part 2”

  1. The move ctor is moving the points from the other-object and loading the area from the atomic member of the other object, but those two actions aren’t performed in an atomic way. Couldn’t the area change after the points got moved but before the area gets loaded concurrently? Wouldn’t it be safer to either invalidate the area or am I missing a point?

  2. @Felipe – some people (ie John Lakos) would say you are talking about about the *salient attributes* of the object. *You* (class author) decide which aspects of an object are salient and which aren’t.

    This is typically the observable properties, but not always – eg capacity() is observable for vectors, but it is not considered part of its ‘value’ and thus not part of its equality operator.

    You are definitely on the right track – mutable things are typically implementation details and typically not part of the object’s _value_, and thus not part of its equality operator.

    I think calling GetArea() inside equality is fine, particularly if the implementor thinks (measures!) that’s a good trade-off.

  3. I guess I’m going into a very different subject here, but the original version of this item got me thinking about the difference between the state of an object and equivalence between objects. I’ve been looking for a good forum to put this out for discussion. What follows more or less reflects my train of thought.

    Consider the following client code, assuming Polygon provides {operator==} and {operator<<}:

        Polygon p1, p2;

    p1.AddPoint( Point(0.0,0.0) );
    p1.AddPoint( Point(1.0,0.0) );
    p1.AddPoint( Point(1.0,1.0) );
    p1.AddPoint( Point(0.0,1.0) );

    p2.AddPoint( Point(0.0,0.0) );
    p2.AddPoint( Point(1.0,0.0) );
    p2.AddPoint( Point(1.0,1.0) );
    p2.AddPoint( Point(0.0,1.0) );

    std::cout << "The area of p1 is = " << p1.GetArea() << std::endl;
    std::cout << "Are they equal? " << ( (p1==p2)?"Yes!":"No :(" ) << std::endl;
    std::cout << "What do they look like?\n";
    std::cout << "p1: " << p1 << std::endl;
    std::cout << "p2: " << p2 << std::endl;

    The objects p1 and p2 are certainly equivalent, i.e. they represent two identical polygons, but after the call to {p1.GetArea()} (which happens to be a const member function that alters a mutable private member variable) their state is different. My questions are:

    1. What should {operator==} test for?
    2. What should {operator<<} print?

    My own answers:

    1. {operator==} should test for equivalence, and thus never compare a mutable member variable (is that a good guideline?). However, should one call {GetArea()} in the body of {operator==} and thus possibly change the object's state?
    2. {operator<<} should print all the observable attributes of the object, not its state (i.e. printing the value of {GetArea()}, not the current value of the mutable variable that represents the area). One could, in fact, provide a separate method (perhaps called {serialize()}) that would print the state in case that's meaningful.

    So seems that my approach distinguishes between 'observable state' and 'implementation state', and the two operators should reflect the former. I then wonder, why not make them always non-member (free), non-friend functions?

    A few other questions come up as a consequence of my own answers, but I'll leave it at that and hope for some comments from more illuminated folks.

    Disclaimer: The actual mechanics of comparing 2 polygons can get as complicated as one wishes, I'm just taking a naive, intuitive approach and don't really care about the innards, mostly the interface. Also, apologies in advance if this rant is way off topic.

  4. I don’t understand why you are setting area in the move ops, but setting it to -1 in the copy ctor/assignment.

    I see it still works either way, but since the whole idea of caching and delaying the area calculation is for the sake of optimization, why not avoid it here as well?

    Also, I like the idea of using memory_order_relaxed for setting area on the non-const operations. But then again, I’m forced to wear atomic kevlar regularly, so I’ve gotten use to it, and the rash has nearly cleared up…

  6. Wouldn’t the move operations need to set other.area to -1 since a cached area would no longer apply to the now-empty points vector?

  7. @Fernando: I just backed out that .load — whether or not you transfer other’s cached area is just an optimization. And by removing it from the code, it helps to answer your other question, because it emphasizes that you don’t need to move the atomic for semantic correctness, and if you used a mutex you wouldn’t need to move that either: Just as when you move you don’t get a new atomic (it’s still the same atomic for this instance) you also don’t get a new mutex (it’s still the same as before). The only thing you have to move is the logical state, which is the vector of points, and set the cached area to a valid value or -1, then you’re consistent.

    So with a mutex, the move would look exactly the same as I’ve now updated it to be above — just move other.points and reset area:

    class polygon {
public:

    /* ... all the other stuff ... */

    polygon( polygon&& other ) 
        : points{move(other.points)}, area{-1} { }

    polygon& operator=( polygon&& other ) {
        { points = move(other.points); area = -1; return *this; }

private:
    vector<point> points;
    mutable mutex mutables;
    mutable double area;
};

  8. Hi Herb.
    GCC 4.8 and Clang ( svn version ) works as expected. :)
    I never thought the solution using .load(), great!
    Some time ago I had a similar problem, but using std::mutex.
    The only solution that came to mind (to make my class movable) was using the Pimpl Idiom, but that involves allocation of memory in the free-storage :(
    Do you have any way to implement a similar solution but using std::mutex as a member?

  9. @edflanders: Yes, because you then take by value + move(param) as Arne noted. The only case where that’s not a good idea is when you have a type that’s expensive to move, such as std::array. I currently plan to cover this in a new GotW on general parameter passing, probably sometime in June or July.

    @Elvis: That wouldn’t allow area to be non-mutable. It’s mutable because it can be written to in const operations, which is still true if you restructure the code the way you suggest. As long as there’s a cached area that can be written to in const operations, it needs to be mutable, and synchronized as atomic or with an internal mutex.

  10. @Leo: Aha, thanks for spotting that leftover text from the original GotW. In several original GotWs I used to recommend considering returning a const value, instead of a non-const value, on the basis that it would help callers by letting them know when they accidentally modify a returned rvalue. I no longer think that’s good advice, especially since you want to move from rvalues which is a non-const operation, so I’ve been rooting it out of the GotWs as I go through, and I got rid of most of the references to that in this one but missed that one. Thanks, fixed.

  11. @Fernando: Sigh, you’re right. You know, that was my reading too and I wrote them out, then noticed that GCC let me get away with =default for the move operations and didn’t reread the standard. Since I had already written them out before switching to =default, the fix was as easy as hitting Ctrl-Y twice. :) Fixed mo betta.

  12. @Arne Mertz:
    Thanks for the response. What about classes that don’t benefit from move? I understand that, for a class like the one below, there will still be two copies despite of the use of std::move()?

    struct Matrix3x3
{
  double m_[3][3];
};
typedef Matrix3x3 T;

  13. void calc_area() const

    in my opinion must be 

     double calc_area() const

    in this way we can avoid mutable area.

  14. @edflanders: 

    struct S
{
  S(T t) : t_(std::move(t))) {}  // at most one copy, one or two moves
  void SetT(T t) { t_ = std::move(t); }  //at most one copy, one or two moves
  T t_;
};

    It’s in both cases one copy, one move, if the argument is an lvalue, and two moves, if it’s an rvalue. In the setter, t is move-or-copy _constructed_, while t_ is move-assigned.

  15. In old GotW 6, there is a guideline said:
    “Guideline: When using return-by-value for non-builtin return types, prefer returning a const value.”

    no longer true? If it is still prefered, get_point should return const point, right ?

    const point get_point( int i ) const { return points[i]; }

  16. Regarding the guideline “Prefer passing a read-only parameter by value if you’re going to make a copy of the parameter anyway…”:

    Does that apply to cases where need to copy to a specific location? E.g.:

    struct S
{
  S(T t) : t_(t)) {}  // two copies?
  void SetT(T t) { t_ = t; }  // two copies?
  T t_;
};

    Does the guideline hold, or should you still pass T const& in these cases?

  17. “Incidentally, notice that once we make rhs a reference-to-const parameter as noted above, we see another reason why get_point should be a const member function, returning either a const value or a reference to const.”

    Currently get_point returns neither.
    Is the idea behind a const value return type to prevent erroneous code like

    int i ...
poly.get_point(i) = ...
poly.get_point(i) += ...

    from compiling? But when assigning to a variable the top level const would be ignored and the variable could be used without constraints?

  18. @Herb: You wrote: “@Francisco: You wrote: “The answer contains good wisdom on const correctness, but got confused about the concurrency semantics meaning/correctness…” – could you elaborate on what you mean?” – First, I meant: “I got a little confused”, not the article, and, it’s nothing all special but just what one can derive from the general reaction to the article. The things related to internal/external synchronization, I think it may have not been explicit enough for the layperson, even more for the ones used to externally synchronized libraries. I wait for the synchronization wisdom from the mentioned GOTW to deal with this api contracts, etc.

  19. Hi Herb,

    hummmm… I think that “move-ctor = default” is not going to help.

    I don’t see anything abount std::atomic’s move-ctor in the Standard ( n3691 – [atomics.types.generic] )

    If atomic copy-ctor is explicitly deleted, the, the move-ctor is deleted too ( implicitly ).
    Am I right?

    I expected to find something like …

    atomic(atomic&&) = default;

    … in the Standard, but I didn’t find it.

    All this makes it very difficult to create classes with std::atomic or std::mutex as members.
    Is it worth another GotW? :)

  20. @Herb: Fair enough. Out of curiosity, are you concerns similar to those in the SO link (interaction with uniform initialization) or are there some other reasons? // Huh, perhaps brainstorming on this could make an interesting (JG?) question on its own…

  21. @Fernando: You are correct, thanks. I’ve now added the copy and move operations; fortunately I could use = default for the latter.

  22. You wrote on operator+:
    “If the caller passes a temporary polygon object (an rvalue), the compiler automatically move-constructs lhs from that, which probably makes no difference for a small type like polygon but can be considerably cheaper for many types.”

    polygon may be a small type but it still contains std::vector as a member. And move-constructing that could make a significant difference in some use cases, couldn’t it?

  23. @mttpd: I went back and forth on that one several times, and several drafts had emplace_back. I stuck with push_back only because I wasn’t (yet) fully confident that emplace_back is a full replacement and should be used by default, but that may well be where we land and if so I’ll take a pass to purge push_backs out of the GotW code.

  25. @Herb: Isn’t memory_order_relaxed still stronger than what we actually need here? We don’t even need atomicity here, is that right?

    I’m mostly trying to understand where the synchronizations are needed in the world of “multiple reader/single writer”.
    This convention shakes my world a little. Until now, I respected (some might say “tried to respect” :)) the rule that if a variable is synchronized through atomic/interlocked operations or through mutex in one part of the code, it should be synchronized everywhere.
    With your convention, non const methods generally don’t need synchronization at all. If the rule is as simple as “if you don’t write const, you don’t need atomic/mutex access”, then I think wearing Kevlar glove while typing the non synchronized code might be overkilled. If the rule is that simple, what are you concerned about?

  26. @Vincent: In that case, yes, you could probably get away with writing area.store(-1,memory_order_relaxed); but I’ll tell you why you shouldn’t. First, in general, you should ban explicit use of memory_order_* by default in your coding standards and permit it only on a case by case basis with rigorous performance data justification and correctness review by memory model experts. Second, it’s unlikely you’ll get performance data justification here because right after “area = -1;” comes “points.push_back(pt);” which is likely to dominate (possibly even if it doesn’t do an allocation).

    When you see someone tempted to hold a lit firecracker or an explicit memory_order_* in their hand, really be highly suspicious and rigorously measure to see if it’s a screaming problem that makes this necessary, and then most of the time make them put it down, and in rare cases approved by certified bomb squad experts allow it but make the person wear a Kevlar glove and maybe a little body armor.

  27. To follow up on my question on how much needs to be synchronized, the atomic solution (which most likely has the smallest overhead, at least in term of space) has the same implementation as the non synchronized version for add_point:

     void add_point( const point pt ) { area = -1; points.push_back(pt); }

    This calls atomic::operator = (double) and can be rather expensive (hundreds of time more expensive than assigning -1 to a double?). Because add_point is non const, this is an unnecessary expense.
    When using the mutex solution, it’s “easy” to remove the synchronization where it’s unnecessary (just remove the lock_guard). Is there a way to remove the “atomic” here? to do a non synchronized store to area?

  28. @Francisco: You wrote: “The answer contains good wisdom on const correctness, but got confused about the concurrency semantics meaning/correctness…” – could you elaborate on what you mean?

    @Vincent, Phil: My comment in Option 1 meant that other const functions that use area, even just to read it, also need to take the lock. Here, that is just get_area which as Phil wrote performs a read of area and so could see a torn read if not also synchronized; that’s why I wrote the note. However, I can and should say this all more simply here to avoid this confusion: Since in this case that’s the only other const function that uses area, and it calls get_area, for simplicity and completeness the easiest thing would be to move the lock there and then I can remove the note. Updated, thanks!

    @jlehrer: Well, it’s transitively correct, but yes that text would work better one paragraph later in calc_area. Done, thanks.

    @Motti: Thanks for pointing out I had two 6 points. Fixed. (However, the revised class at the end is perfectly conforming to normal thread safety and a model of how to do it, see above comment.)

    @several asking why I didn’t make various suggested improvements not related to const: Because the problem statement said not to do drive-by improvements, but focus on const issues. :)

  29. @Steve: There’s nothing “blindly” about it, it has to be quite eyes-wide-open and intentional. “Blindly” would be having a mutable member variable and not synchronizing it. :) The reason is that the caller, who must perform the usual correct external synchronization for non-const accesses if the variable is shared, cannot do the right thing for concurrent const accesses because he doesn’t know about the internal state shared among const accesses, therefore you need to do internal synchronization to get back up to the normal level of thread safety. (This is a general issues and one of the huge pitfalls of copy-on-write (COW) as I wrote about before.)

    As mentioned in GotW #6a, unless you can prove that no object of your type will ever be shared (generally impossible or infeasible), you have to choose the tradeoff you like best: Don’t have mutable variables that could be written to in const member functions and don’t add internal synchronization (at least not for this reason), or have mutable variables that could be written to in const member functions and add internal synchronization. If you’re going to have mutable state like this cache, that is an internal implementation detail not visible to the caller but that would perform writes in const member functions, you need to synchronize it as discussed in 6a and 6b.

    @Rick, Paul, Craig, Alb: As Rick’s and Arne’s and Erik’s followups noted (Erik summarized it particularly well), the revised polygon is fine and recommendable, it’s the caller who wrote a bug – he wrote a race by modifying the variable via .add_point on one thread and concurrently using it on another thread without external synchronization. The definition of a race condition is “two concurrent accesses and at least one is a writer” (i.e., non-const).

    All this is making me think that I should write an additional GotW I wasn’t planning on to discuss what “thread-safe” means, and discuss the difference between internal synchronization and external synchronization and when each is necessary. It’s a perennial question that’s actually mostly language-neutral but it seems there needs to be a treatment of it, including things like why constructors and destructors generally never need to perform internal synchronization (many people don’t realize this).

    @Martin: Yes, these are language-neutral concurrency questions. The only C++-specific details are the spelling of things like unique_lock, mutable, and const.

  30. I would have expected some advice here on the appropriate uses of const for the copy constructor, copy assignment, and move assignment operators, unless the fact that this class has mutables and/or atomics has no impact on how const is used for those methods..

  31. @Phil Miller: Read the text below the Option 1 example. Herb points out that you would need to grab the mutex everywhere you access area. So, although he didn’t write it out, it’s clear that you’d need to modify get_area.

  33. I believe there’s a bug in the described mutex version of calc_area() as it interacts with concurrent calls to get_area():

        void calc_area() const {
        auto lock = unique_lock<mutex>{mutables};
        area = 0.0;
        for( auto& pt : points )
            area += /* some work using pt */;
    }

    double get_area() const {
        if( area < 0 )   // if not yet calculated and cached
            calc_area();     // calculate now
        return area;
    }

    These are both const, so they should be callable completely concurrently. However, a second call to get_area() which a first one is running can return wrong results, because it will see intermediate non-negative values of area:

    Thread 1:                                 |  Thread 2:
d1 = poly.get_area();                     |
> cal_area() called                       |
auto lock = unique_lock<mutex>{mutables}; |
area = 0.0;                               |
// transient non-negative state of area   | d2 = poly.get_area();
for( auto& pt : points )                  | // check against 0, return
  area += /* some work using pt */;       | // any time here
> calls return, d1 is correct             | // value in d2 is arbitrary

  34. Let ‘s say that the fact is that I have been seeing code only protecting mutations locally without wondering of the entire thread safety inside the application. This local protection were often considered as to having done the job for taking into account the concurrent issues. That’s why I think they give a false sense of confidence against these problems.

    I tend to think that if internaly the class protects its mutable states against concurrency issues then the class shall be ‘externaly’ entirly immutable, i.e. all the public and protected functions shall be const and all members shall be either const (const vector points;) or mutable (such as mutable atomic area). Function add_point shall no exists as a member one but as an external function creating a new immutable polygon object.

    On the other hand, if the class enables mutability of its instance by their users, it is impossible to assume that this mutability won’t concurrently happen while a const function is running. How can you ensure that the code of the working threads accessing the shared object only in a read-only way will still be modifiable when adding new threads that uses these objects in a non-const way ?

  35. You know … reading these examples always makes me (genuinely) wonder: Is it C++ that we “have to” think about all these subtleties, or could one also write such articles for other languages?

    In other words: Are there any good GotW’s for C# (or Java)?

  36. Wrt to the thread safety of add_points and calc_area, the idea is to provide a normal degree of thread safety, not a complete lock strategy for the class. Synchronizing for mutability is (as always) best done externally to the class.

    What Herb does is just satisfying the normal expectancy that const-only (ie, thought of as read only) access is concurrency-safe.

    An example: If I produce a polygon in a thread, and then make that visible (thread safely) to a bunch of workers doing const-only access, I’ll not bother synchronizing access to the polygon in the workers. I’m only reading it!
    Without the atomic, get_area will race on the first accesses.

    If I changed the design to having the workers mutate the polygon, then I’d add a locking strategy for it. Probably a mutex that each worker must hold before even looking at the poly.

    The polygon class without the atomic will be a nasty surprise for me as a user, with the atomic it’ll behave as expected and let me use my standard patters for concurrency without nasty surprises.

  37. Hello,

    * What happen if add_point is called concurrently while executing the loop processing the area ? if I remember correctly, the push_back call may invalidate iterators.

    * What about the get_area returned if add_point is called concurrently just after “area = tmp;” statement in the calc_area function and before the return statement of the get_area function ? Don’t it will return -1 ?

    Even if point is constant, shall not it be considered to be protected against concurrency since it is mutable in the class point of view ?

    Does not atomic (and mutexes and all this kind of local synchronisation patterns) give a false sense of confidence agains concurrent issues ?

  38. calc_area() iterates over the points collection without protection. If execution of one thread is in this loop while another thread calls add_point() then calc_area will fail with undefined behavior as the collection is ripped out from under it.

  39. As Rick Yorgason said this class is not thread safe. In my opinion it’s even worse than what he points out.
    If a thread calls add_point while another thread is calculating the area the `points` vector may be reallocated thus invalidating all iterators. In which case `calc_area` causes undefined behaviour!

    In other news, you have two “6” points.

  41. @Rick (and Paul): right, the concurrent call of add_point is not threadsafe, as is any concurrent call of modifying (read: nonconst) functions on an object.

    @Steve: I am surely not an expert, but the only overhead I know of in std::atomic are the limitations on optimizations the compiler may do on write access. Since that write access is very seldom (once in calc_area, wich is called only after changing the shape), the overhead should be very close to negligible. Any policy based switching to non-atomics for non-concurrent usage would be overkill wrt the implementation, it would make the code a tiny bit less readable and it would introduce the danger of picking the wrong implementation in multithreaded environments or to simply forgetting to switch when changing from single threaded to multithreaded. And since almost every system is parallel these days you shoud just not code for singlethreaded environmets explicitly any more.

  42. Why don’t you use std::accumulate instead of the hand-crafted for-loop in point 5? I think it is more expressive than writing a for-loop:

    Combining all that, we get this simpler and const-correct code:


    for( auto& pt : points )
    area += /* some work using pt */;

    becomes

    void calc_area() {
    area = std::accumulate( std::cbegin(points), std::cend(points),
    0,
    [](double x, double y) {return /* ... */} );
    }

  43. @jlehrer – what about mixed use? We often have data structures which make sense in both single- and multi-threaded environments. Often both usages crop up in the same application; and as such, having to pay the multi-threaded penalty in the single-threaded domain doesn’t make me feel nice!

    Perhaps making polygon a class template would be deemed too cumbersome? Perhaps it is premature optimization? I don’t know, but in any event a lock policy solves this.

  44. In response to Steve Lorimer’s comment about single threaded versions of the code – If I were designing a library for public use I would probably use a typedef for this atomic variable that switched between an atomic double and a plain old double. I would allow the consumer to specify via macros if they wanted to compile in multi-threaded support or not.

  45. You wrote:

    “Once you make [get_area()] const, the compiler will tell you that you also need to do something about the member variable area”

    No, it won’t. The compiler will only tell you that you are calling a non-const method from within a const method. The member variable “area” is not modified in this function, only read. I suspect that at one point you did not have the “calc_area()” method and did the work inline in “get_area()”. When you extracted out the calculation and storage to get_area() you probably didn’t update the explanation text.

    Or, you have a method called “calculate_area_impl” the was non-mutating and returned the calculated area that was then stored inside of calc_area().

    Either way, the body text of the solution, I believe, is incorrect.

  46. I had more-or-less the same thought as Rick (post #4), but my concern was that thread 1 could be stepping through the collection of points at the same time as one or more points are being added to it, producing an indeterminate result, as the thread-safety is only introduced when the result is written to the member variable. I haven’t given it a lot of thought, but it appears to me that the mutex approach provides a means to (partially) overcome this. OTOH, I think that this class is clearly not thread-safe, and I guess that the point of GOTW #6b is not to address the wider issue of thread-safety for this poorly designed class, but I feel that the example is a bit limited.

  48. @Herb: Yay! :-)

    To give credit, after verifying with GCC (http://ideone.com/KcGaII), I’ve also checked this on SO: http://stackoverflow.com/a/3802028/

    True, there are “some compilers [that] do not flag const references”:
    https://www.securecoding.cert.org/confluence/display/cplusplus/DCL33-CPP.+Never+qualify+a+variable+of+reference+type+with+const+or+volatile

    To my understanding, however, justifying something like that would require a rather (too) liberal reading of the phrase with the conditional “ignored” allowance, treating it instead as a general (unconditional) escape valve: basically, ignoring (no pun intended) the “typedef/template type argument/(new to C++11)decltype-specificer” restriction.

    // On a side note, the conditional allowance for template type arguments might have been a necessary inconsistency:
    http://groups.google.com/group/comp.lang.c++/browse_thread/thread/8e82c3edb2a9a645/

  49. Herb, you mention “// Option 1: Use a mutex (NB: Also lock in other funcs that use area)”.
    Given that add_point is not const and hence is not thread safe (by design), why would a mutex be needed there? Couldn’t the mutex be just skipped in that method (but kept in calc_area obviously)?

  50. Actually, I guess the root of the problem is that add_point isn’t thread safe, but since it’s not const, you’re not guaranteeing that it is.

  51. I believe there’s still a bug with that atomic variable. Consider this code:

    Thread 1:
d1 = poly.get_area();

Thread 2:
poly.add_point({1, 1});
d2 = poly.get_area();

    If that gets executed like so:

    Thread 1:             | Thread 2:
d1 = poly.get_area(); |
> calc_area() called  |
                      | poly.add_point({1, 1});
                      | d2 = poly.get_area();
                      | > calc_area() called
                      | > area = tmp executed
> area = tmp executed |

    then d2’s area does not include {1, 1}.

  52. Herb, you mention above that area should be synchronized using a mutex or made atomic, so that it’s concurrency-safe, as discussed in GotW #6a. – this introduces overhead which would be otherwise avoidable in a single-threaded scenario.

    Do you think blindly using mutexes / atomics whenever you have a mutable variable is the correct approach? Could doing this excessively lead to performance issues?

    Perhaps using Andrei Alexandrescu’s approach of policy based design to specify whether polygon is used in a single- or multi-threaded environment. In this way we could have locking / atomically updating area be a no-op when in a single-threaded environment.

    Or is what I am suggesting premature optimization?

  53. @mttpd: I do believe you’re right. I see that same wording all the way back in C++98 too, and I first wrote that some 16 years ago and as far as I can recall you’re the first to point this out. It probably slipped by because a number of compilers I’ve tested against over the years did accept the const as redundant which is a conforming (if useless) extension. Fixed, thanks. And you get those bonus points!

  54. > “(If, in looking for the “Bonus” points, you said something about these two functions being uncompilable—sorry, they’re quite legal C++. You were probably thinking of putting the const to the left of the & or *, which would have made the function body illegal.) ”

    I don’t think so, per 8.3.2/1 this use of cv-qualified references is ill-formed:
    “Cv-qualified references are ill-formed except when the cv-qualifiers are introduced
    through the use of a typedef-name (7.1.3, 14.1) or decltype-specificer (7.1.6.2), in which case the cv-qualifiers
    are ignored.”

Comments are closed.