| ||||
| Comment on GotW #5 Solution: Overriding Virtual Functions by Bhupesh Pant
@Herb, @Alok & Sil, I totally agree with what you are saying but remember Rome was not build in a day and so will your dream language. One thing I am sure you could feel the change. Some very smart people is constantly working hard for this change, probably we could see some major changes in C++14 or C++(yet to come) … :) Read More » Comment on GotW #5 Solution: Overriding Virtual Functions by Rick Yorgason
The one thing I’ve found final useful for is for marking when functions in base classes have been renamed or are otherwise obsolete. For instance: class Foo { protected: virtual void bar() final {}; // Don't use this anymore. Use baz() instead. virtual void baz(); }; Read More » Comment on GotW #6b Solution: Const-Correctness, Part 2 by Herb Sutter
@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! Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by mttpd
> “(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: Comment on GotW #6b Solution: Const-Correctness, Part 2 by Steve Lorimer
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? Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Rick Yorgason
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. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Rick Yorgason
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}. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Vincent Lascaux
Herb, you mention “// Option 1: Use a mutex (NB: Also lock in other funcs that use area)”. Comment on GotW #89: Smart Pointers by Adam Burry
And, should you pass shared_ptr by value or by reference? Read More »Comment on GotW #5 Solution: Overriding Virtual Functions by Michael Urman
@rhalbersma as an example of such a scoped guard see GotW 6b, part 6, option 1. { auto lock = std::unique_lock<mutex>{mutables}; ::: // no further references to lock // lock destroyed; mutables released } Read More » Comment on GotW #6b Solution: Const-Correctness, Part 2 by Francisco Lopes (@pepper_chico)
The answer contains good wisdom on const correctness, but got confused about the concurrency semantics meaning/correctness… Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by jlehrer
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. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by jlehrer
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. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by skebanga
@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. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Jens
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:
becomes Comment on GotW #6b Solution: Const-Correctness, Part 2 by mttpd
@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”: 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: Comment on GotW #6b Solution: Const-Correctness, Part 2 by Paul Byrne
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. Read More »Comment on GotW #89: Smart Pointers by Anonymous
Regarding #4, I assume the eventual advice will include things like being able to banish “naked new” as a pedagogical point, and maintaining exception safety when there’s multiple arguments, e.g. void f(unique_ptr<Foo>, unique_ptr<Bar>); f(new Foo, new Bar); // leaks if the first ctor called -- whichever that is -- throws But, IMHO, these result in a much less persuasive argument for *always* using make_unique compared to the situation for make_shared which is much more cut and dry, especially since make_unique was left out of C++11. Yeah, I know it’s only a few lines and it’s in C++14, but as long as you aren’t passing multiple arguments and you aren’t writing example code for a book, I’m not seeing the real burning need for make_unique. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Arne Mertz
@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. Read More »Comment on GotW #89: Smart Pointers by Rick Yorgason
I like make_shared and make_unique. They make my code safer and shorter. Usually. The exception is when you reset a smart pointer. smart.reset(new SomeType(val1, val2)); is shorter than smart = std::unique_ptr<SomeType>(val1, val2); And I know that’s safe, so it undercuts the “Never use new” mantra a bit. It would be great if the library supported this: smart.emplace_reset(val1, val2);Read More » Comment on GotW #6b Solution: Const-Correctness, Part 2 by Thibaud
What about std::call_once to compute area ? Read More »Comment on GotW #89: Smart Pointers by Rick Yorgason
Blah, brain fart. That second code block should read: smart = std::make_unique<SomeType>(val1, val2);Read More » Comment on GotW #6b Solution: Const-Correctness, Part 2 by Craig Henderson (@cdmh)
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. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Alb
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 ? Read More »Comment on GotW #89: Smart Pointers by Olivier
I’ll give it a small try. 1) One should use shared_ptr when the ownership of a resource is shared between several objects and the order of destruction of these objects is not known at design-time. 2&3) The make_x() functions are preferable over manual new + creation of a smart pointer because 4) auto_ptr never existed. These aren’t the droids you are looking for. *waves his hand*. :) Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Erik
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! 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. Read More »Comment on GotW #89: Smart Pointers by pip010 (@ppetrovdotnet)
1: depends on whether you have a clear owner or not of a resource Comment on GotW #6b Solution: Const-Correctness, Part 2 by Martin
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)? Read More »Comment on GotW #89: Smart Pointers by j
1. when shared owner ship is desired the shared_ptr can be used, otherwise default to unique_ptr //if either fails, then a leak could occure f(new int(3), new double(5.6)); //if either fails all resources will be cleaned up f(make_unique(3), make_unique(5.6)); 4. auto_ptr is depricated Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Phil Miller
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 Read More » Comment on GotW #6b Solution: Const-Correctness, Part 2 by Alb
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 ? Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Motti Lanzkron
As Rick Yorgason said this class is not thread safe. In my opinion it’s even worse than what he points out. In other news, you have two “6″ points. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Fernando Pelliccioni
Herb, You need to provide a move-ctor or a copy-ctor to polygon because … atomic(const atomic&) = delete; Same with std::mutex Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Adrian
@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. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Adrian
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.. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Herb Sutter
@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. :) Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Herb Sutter
@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. Read More »Comment on GotW #89: Smart Pointers by Adrian
I’ve a question that I hope is covered in the solution. make_shared enables an optimization in that it can allocate the control block along with the actual object. Presumably this improves locality, and could even eliminate one memory allocation. But what happens if the class being instantiated has class-specific new and delete operators. Will make_shared use that one (and thus still make a separate allocation for the control block)? Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Vincent Lascaux
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. Comment on GotW #6a Solution: Const-Correctness, Part 1 by artur
gcc 4.8 -std=gnu++11 http://en.cppreference.com/w/cpp/locale/locale std::isdigit( jakis_znak, std::locale::classic() ); valgrind: ==62666== This conflicts with a previous write of size 8 by thread #3 Comment on GotW #6b Solution: Const-Correctness, Part 2 by Herb Sutter
@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. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by mttpd
A nitpick (and possibly very much in the “drive-by improvement” category ;]) — I would suggest replacing all occurrences of “push_back” with “emplace_back” — as of C++11 it may be a good idea to consider it the default choice. Perhaps it’s even fair to say it’s debatable whether “push_back” should be used at all anymore: Comment on GotW #6b Solution: Const-Correctness, Part 2 by Vincent Lascaux
@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”. Comment on GotW #89 Solution: Smart Pointers by Herb Sutter
BTW, the reason I’m posting #89 and #90 in faster succession is because we already saw versions of these exactly one year ago, as then-numbered #103 and #104, so some of the comment discussion about them would be duplicated. However, I think you’ll see both have been considerably updated since the initial solutions I posted a year ago — and not just because of make_unique, and its corollary don’t-write-new, but at least one other C++14-ism in the one to come next. Read More »Comment on GotW #6b Solution: Const-Correctness, Part 2 by Herb Sutter
@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. Read More » | ||||
| ||||
Wednesday, May 29, 2013
FeedaMail: Comments for Sutter’s Mill
Subscribe to:
Comments (Atom)