JG Question
1. In each of the following statements, what can you say about the order of evaluation of the functions f, g, and h and the expressions expr1 and expr2? Assume that expr1 and expr2 do not contain more function calls.
// Example 1(a) // f( expr1, expr2 ); // Example 1(b) // f( g( expr1 ), h( expr2 ) );
Guru Questions
2. In your travels through the dusty corners of your company’s code archives, you find the following code fragment:
// Example 2 // In some header file: void f( T1*, T2* ); // At some call site: f( new T1, new T2 );
Does this code have any potential exception safety or other problems? Explain.
3. As you continue to root through the archives, you see that someone must not have liked Example 2 because later versions of the files in question were changed as follows:
// Example 3
// In some header file:
void f( std::unique_ptr<T1>, std::unique_ptr<T2> );
// At some call site:
f( std::unique_ptr<T1>{ new T1 }, std::unique_ptr<T2>{ new T2 } );
What are the semantics of this call? What improvements does this version offer over Example 2, if any? Do any exception safety problems remain? Explain.
4. Demonstrate how to write a make_unique facility that solves the safety problems in Question 3 and can be invoked as follows:
// Example 4 // In some header file: void f( std::unique_ptr<T1>, std::unique_ptr<T2> ); // At some call site: f( make_unique<T1>(), make_unique<T2>() );
Sutter’s Mill 
@Xeo did you explicitly omit a new expression in the the return statement?
Is that a new feature of std::unique or just a typo?
Sheesh, damn markup. Let’s try that again for the make_unique code:
#include <memory> #include <utility> template<class T, class D = std::default_delete<T>, class... Args> std::unique_ptr<T,D> make_unique(Args&&... args){ return std::unique_ptr<T,D>{ std::forward<Args>(args)... }; }@Jonathan, @Andrzej:
Never return a T&& when you actually return a temporary. This is the same issue as this one:
int const& f(){
return 5;
}
int main(){
int i = f();
}
And as MSVC10 rightly warns me here, you are returning a reference to a temporary here. This doesn’t change with rvalue references, they are still just references. Just leave it as is, with a value return.
On Topic:
#1 a: In C++11 terms, both expr1 and expr2 are sequenced before f, but are unsequenced relative to each other, i.e. they can both be evaluated in any order.
#1 b: Not really that different. expr1 and expr2, aswell as g and h are sequenced before f, while expr1 is sequenced before g and expr2 is sequenced before h. I’m not totally sure about this last part, but all other cases should be unsequenced, e.g. expr1 could get evaluated before h, or it might aswell get evaluated after expr2.
#2: Same as #1 a, just with actual expressions instead of placeholders (new T1 == expr1, new T2 == expr2).
#3: Same as #1 b really (new T1 == expr1, new T2 == expr2 and std::unique_ptr<T1>{ expr1 } == g, std::unique_ptr<T2>{ expr2 } == h.
#4
#include
#include
template<class T, class D = std::default_deleter, class... Args>
std::unique_ptr make_unique(Args&&... args){
return std::unique_ptr{ std::forward(args)... };
}
Now, #4 is again the same as #1 a w.r.t. sequencing, but it has an important difference: The new expressions are explicitly sequenced, since they are part of a function call. They can’t be magically sequenced before actually being bound to a std::unique_ptr.
(I hope that I got the standard terminology right.)
I’ll take a stab at this…
1. The order of evaluations is unspecified. C99 allows expr1 and expr2 to be evaluated simultaneously. (Not sure what C1x, C++03, or C++11 say, but I’ll assume the answer is the same.) I believe on x86 platforms the latter arguments get evaluated first, but that is a mere implementation detail :)
CERT addresses this issue in some depth (and I’d like more!) in the CERT C & C++ rules:
EXP30-C. Do not depend on order of evaluation between sequence points
https://www.securecoding.cert.org/confluence/x/ZwE
EXP30-CPP. Do not depend on order of evaluation between sequence points
https://www.securecoding.cert.org/confluence/x/fYAyAQ
IIRC WG14 wanted to eliminate sequence points from C1x but it never happened.
The examples raise more interesting questions though. While you said nothing of separate execution threads, the problem of determining which of two threads does execution first is very similar to the problem of which expression gets evaluated first. If expr1 and expr2 did both manipulate some global variable, you could have race conditions on the variable. I suspect this bears partly on your next question.
I’ll conclude up by saying that this is not a problem in Java because Java explicitly specifies that the first expression in a function argument list finishes being executed before the second begins. That saves Java programs from most (all?) of the headaches this puzzle is about.
2. The code definitely has some problems and probably has others. As pointed out above, if there is insufficient memory to allocate both objects, then a bad_alloc exception can be thrown while the other object is successfully allocated, causeing a memory leak. But even if operator new never fails, I don’t think this code is safe, or is at best non-portable. As noted earlier, the order of the operator new’s is unspecified. While operator new’s implementation is also unspecified, I’m not convinced that it’s safe to have these two invocations that could (theoretically) be intertwined. I’m pretty sure that its unsafe to have two threads invoke operator new simultaneously under C++03 and POSIX threads. Not sure if Windows threads or C++11 threads permit this.
3. The code tries to mitigate the first problem cited above and fails, because of the second problem. Wrapping the new expressions in unique_ptr solves the memory leak. But not in the way the code is done because the order of evaluation is still unspecified.
4. This code would have mitigated both problems:
{
// At some call site:
auto t1_ptr = std::unique_ptr( new T1 );
auto t2_ptr = std::unique_ptr( new T2 );
f( t1_ptr, t2_ptr);
} // both ptrs released
I don’t think you can mitigate the problem using a single line of code that looks like f(… , …). Well, maybe you could if you use static/global variables, but the code would certainly be ugly and your manager would shoot you if he saw such code in production software :)
I put the ellipsis in the wrong place. Here’s a revision:
template < typename Type, typename... Args >
typename std::enable_if< std::is_array<Type>::value == false, std::unique_ptr<Type> >::type
make_unique(Args&& ...args)
{
return std::unique_ptr<Type>( new Type(std::forward<Args>(args)...) );
}
// Array specialization
template < typename Type >
typename std::enable_if< std::is_array<Type>::value, std::unique_ptr<Type> >::type
make_unique( size_t size )
{
return std::unique_ptr<Type>( new typename std::remove_extent<Type>::type [ size ] );
}
Woops, I put the parenthesis around the ellipses in the wrong place… Here’s a correction:
meta-tag. E.g. if that is correct, then you can write, or paste,
std::unique_ptr can hold arrays, too. So if you were to account for that, since template functions do not have partial specialization, we use SFINAE via std::enable_if<>.
template < typename Type, typename... Args >
typename std::enable_if<std::is_array<Type>::value == false, std::unique_ptr<Type> >::type
make_unique(Args&& ...args)
{
return std::unique_ptr<Type>( new Type(std::forward<Args>(args))... );
}
// Array specialization
template < typename Type >
typename std::enable_if<std::is_array<Type>::value, std::unique_ptr<Type> >::type
make_unique( size_t size )
{
return std::unique_ptr<Type>( new typename std::remove_extent<Type>::type [ size ] );
}
…
and the
seems to be preferable because of custom deleter awareness.
#3. It seems that there are more than one right solution here:
unique_ptr<T1> arg1(new T1);
unique_ptr<T2> arg2(new T2);
f(arg1.release(), arg2.release());
and
unique_ptr<T1> arg1(new T1);
unique_ptr<T2> arg2(new T2);
f(std::move(arg1), std::move(arg2));
Thanks Herb for your work ! and sorry for the inconvenience ! Indeed update GotW with the new specification from c++11 is a pretty nice idea !
@Patrick, @Seb: Yes, this is a C++11 update to GotW #56. I’m going to be writing both brand-new GotWs as well as C++11-era revisions of older GotWs.
@Andrzej: Thanks for the explanation. Never heard of xvalue and prvalue (gotta read up on that in the future), but the part “Rvalue reference is still a reference and the same safety rules for references apply” really hit it home for me. I did an experiment with the following code:
T1&& make_T1() { return T1(); } auto t = make_T1();and the invocation sequence is:
1. temporary T1’s constructor.
2. temporary T1’s destructor (the function make_T1() returns a copy of the temporary’s reference and destroys the temporary).
3. t’s move constructor on the return value (which is a rvalue reference).
By the time step 3 is called, the temporary T1 is already gone. Dangling reference. This is what you would expect if the function declaration was:
where step 3 was instead t’s copy constructor.
But with rvalue references I somehow thought the invocation sequence would’ve been 1, 3, then 2. Good to clear that up! Thanks.
@Jonathan: either function:
or
equally well triggers the move semantics: calling the former is an xvalue; calling the second is a prvalue; but xvalues and prvalues are both rvalues which bind to rvalue reference.
However, the variant that returns an rvalue reference is dangerous. Rvalue reference is still a reference and the same safety rules for references apply: you should not return references to automatic objects created inside your functions.
Is it the same problem as this : http://gotw.ca/gotw/056.htm ?
@Alf: I had a similar answer to #4 as you, but instead of returning a plain std::unique_ptr for the make_unique() function, I had the return type as std::unique_ptr&&. That is, my function looked like this:
std::unique_ptr&& make_unique(Args…arg)
Isn’t it better to do that so you get move semantics when you’re calling f( make_unique() )?
I thought you were going crazy and reposted a recent GotW, glad it was just me. I had a problem with one of these at work about three weeks ago and found the solution in your GotW. Turns out it was a similar-but-old one I found in a web search: http://www.gotw.ca/gotw/056.htm. Question: the difficulty dropped from 8/10 to 7/10 – did we all get smarter? I know I did after reading it, thanks Herb for your work in the community.
@Michal: I am not sure if I understand your question, so pardon me if my answer is a bit off-topic. The brace notation is allowed for a number of contexts where the type of the initialized object can be easily deduced:
* return expression,
* assignment expression,
* argument of operator[],
* function call argument:
~~~
struct Rational{ int num, den; };
struct Container{
…
Rational operator[]( Rational r ) {
return {2, 3};
}
}
void test( Rational x );
Rational r1;
r1 = {1, 2};
Container c;
r1 = c[{4, 5}];
test({3, 7});
~~~
@Andrzej, @Herb: That sucks, the notation is allowed for return values right? Seems to be an odd duality there..
@Andrzej: Good catch, thanks. Fixed. I wish that decision had gone the other way; it’s an explicit request to construct something.
Note that example 3 (with shared_ptr and brace-initialization syntax) does not compile in C++11. The new initialization syntax cannot be used this way to trigger the explicit constructor. (At some point the Committee considered allowing this usage, but the final standard prohibits it):
8.5 p. 15:
The initialization that occurs in the forms
T x(a);
T x{a};
as well as in new expressions (5.3.4), static_cast expressions (5.2.9), functional notation type conversions
(5.2.3), and base and member initializers (12.6.2) is called direct-initialization.
12.3.1 p. 2:
An explicit constructor constructs objects just like non-explicit constructors, but does so only where the
direct-initialization syntax (8.5) or where casts (5.2.9, 5.4) are explicitly used. A default constructor
may be an explicit constructor; such a constructor will be used to perform default-initialization or value-initialization
(8.5).
Regards,
&rzej
For others who want to post code in their answers/responses (I’m a bit late discovering things, so my own answer was quite a bit short of perfect!):
This seems to be a WordPress blog, and it seems to support the WordPress
</strong> meta-tag. E.g. if that is correct, then you can write, or paste, <blockquote> <code>[sourcecode language="cpp"] #include <iostream> using namespace std; int main() { cout <<"Hello!" << endl; }and have it render as
#include <iostream> using namespace std; int main() { cout << "Hello!" << endl; }Cheers, & hoping this is deleted if it’s wrong …
@Marat Abrarov:
You wrote:
“
unique_ptr<T1> arg1( new T1 );
f( arg1.release(), unique_ptr<T2>( new T2 ).release() );
has the same problem as #2 and #3.
”
You’re right, thanks.
If the first
release()is evaluated first, andT2::T2throws, one has a leak.However I did not realize that until I had posted, and I could not find any edit button. :-(
By the way, here I used HTML symbolic entities like
<to avoid the collapsing of things enclosed by < and >.Cheers,
> The only right solution seems to be (only because of unique_ptr::release() is nothrow):
> unique_ptr arg1( new T1 );
> unique_ptr arg1( new T2 );
> f( arg1.release(), arg2.release() );
and because of constructor unique_ptr::unique_ptr(T*) is nothrow.
#1 and #2 are trivial.
#3 has the same problem as #2. But declaration of function “f” at #3 has different (better) semantic then #2. At #3 f’s declaration explicitly moves ownership of it’s args to the callee. This is the reason I like smart pointers (and prefer over the raw ones) – an interface with smart pointers seems to be more self-describing. As Boost.SmartPtr best practices says: “Avoid using unnamed shared_ptr temporaries to save typing” (it can be extended to almost every smart pointer).
To Alf P. Steinbach:
unique_ptr arg1( new T1 );
f( arg1.release(), unique_ptr( new T2 ).release() );
has the same problem as #2 and #3.
The only right solution seems to be (only because of unique_ptr::release() is nothrow):
unique_ptr arg1( new T1 );
unique_ptr arg1( new T2 );
f( arg1.release(), arg2.release() );
As I wrote earlier, it’s splendid that you’re continuing the GOTW series.
It would be even nicer if you would allow and encourage general discussion, as you did in former times, by posting these questions to a discussion-oriented forum such as (you did) [comp.lang.c++].
That would also solve the problem of formatting of responses.
That said,
#1:
the arguments of a function call are fully evaluated before a function is called, but apart from that the order of evaluation is up to the compiler. It’s unclear what you’re asking for above that. But as an example, by this rule, for the call
f( g( expr1 ), h( expr2 ) )thegandhcalls occur in some unspecified order before the call off, and for example, both or just one ofexpr1andexpr2may have been evaluated whengis called.#2:
For the call
f( new T1, new T2 );one may have a memory leak if either of theT1orT2constructors throw.“May”: it’s in principle possible to define e.g. class-specific allocation functions that e.g. always allocate the same memory, but that would be pretty unusual and brittle, so let’s ignore that possibility.
A fix for the general case depends on what guarantees
foffers. Assuming thatfguarantees to take ownership of the objects, and guarantees to not leak them if it fails, then all that’s needed is to allocate at least one of them before the function call, and use RAII cleanup. E.g.,unique_ptr arg1( new T1 ); f( arg1.release(), unique_ptr( new T2 ).release() );That looks a bit assymetric, and it is. I don’t know what’s best: to call the reader’s attention to this by intentionally showing the assymetric nature of the thing, or trying to make it look more safe and symmetric by creating both argument objects before the call. I guess, however, that a reader of code will react negatively to such asymmetry, thus it would probably be better to write …
unique_ptr arg1( new T1 ); unique_ptr arg1( new T2 ); f( arg1.release(), arg2.release() );#3:
With
fchanged tovoid f( unique_ptr, unique_ptr )the exact same exception problem remains. And dang if I don’t seem to recall that this has been discussed in some earlier GOTW. Anyway, now as of 2011 it’s a well-known problem, perhaps in part thanks to your GOTW efforts.
The problem is that for an actual call with
new-expressions as actual arguments, the compiler may choose to generate code that evaluates bothnew-expressions before invoking theunique_ptrconstructors.#4:
The question calls for a
make_uniquefacility, and in C++11 that’s either trivial (for the case of a maker function that just addresses the current problem) or quite complex (for the general case).The trivial:
template< class Type, class Deleter = std::default_delete, class... Args > std::unique_ptr make_unique( Args... args ) { return std::unique_ptr( new Type( args... ) ); }The call of
fthen becomesvoid f( make_unique_ptr(), make_unique_ptr() )which is safe, since a function call (here call of
make_unique) must complete before anything else happens.The complexity enters the picture when one wishes support for arrays and for custom deleter values (not just custom deleter type).
One would not, or I would not, want to code up that.
So, it is a bit surprising that this function did not make into the C++11 standard, while `make_shared` did.
#5 (by Alf):
Is this a good solution to the given problem?
It’s ingenious, yes, and it’s a Very™ nice tool to have in general (to wit, the `make_shared` function’s popularity).
As a solution to the given problem it relies on a convention, that whoever calls
fshould remember to usemake_unique. And it’s rather easy to forget to apply conventions, or to apply them incorrectly or only partially. Thus, as a solution to the given problem it’s rather brittle.#6 (by Alf):
But is there any alternative?
Yes, in addition to introducing the
make_uniquefunction (which is very useful in general) the functionfshould be be made safe. It should ideally be made near to impossible to use incorrectly or to use in an unsafe way. And with the originalfas the basic functionality that somehow “must” result, making it safe involves imposing sequencing on the client code’s argument evaluation.Given the freedom the compiler has wrt. argument evaluation such sequencing cannot be imposed with both arguments specified directly.
The problem with
unique_ptrin that respect, is that it allows direct construction of aunique_ptr, without using themake_uniquefunction.There are many possible solutions, including the functional one of replacing f(x,y) with f(x)(y), but one simple solution that also yields fairly concise and readable notation (which I feel is pretty important!), is to just define a wrapper class for
unique_ptr, a wrapper class for this special usage. A separate factory function is then not needed, since a constructor can do that job. It can look like this:template class DynArg { private: std::unique_ptr p_; public: std::unique_ptr value() { return std::move( p_ ); } template DynArg( Args... args ) : p_( new Type( args... ) ) {} DynArg( DynArg&& other ) : p_( std::move( other.p_ ) ) {} };And a wrapper for the original function can then look like
void f( DynArg a1, DynArg a2 ) { original::f( a1.value().release(), a2.value().release() ); }with a call like, for example,
f( DynArg( 1 ), DynArg( 3.14 ) );The wrapper function definition above has the arguments passed by value because that’s most natural when one is familiar with a host of different smart pointers that are passed by value. Still, in C++11 one could perhaps guarantee some micro-efficiency by passing as rvalue references (avoiding moves). I like it simple, though.
1a) f( g( expr1 ), h( expr2 ) );
The order of evaluation is not guaranteed.
2) f( new T1, new T2 );
If T1 is constructed first and If construction of T2 throws an exception, then there is a memory leak here.
3 & 4 not sure of the answers. :-(
I’d guess 3 is still not safe according to the standard, since (again, I guess) it’s valid for compiler to evaluate both news before evaluating the initializer lists, in the same way as it could evaluate expr1 and expr2 before g() and h(). Of course no real-world compiler ever would, since that means you need to keep a temporary around while evaluating a function, *and* it’s more complex to implement.
Though I think C made the right choice for it’s purpose, I really like that C# has extremely strict rules on order of evaluation for parameters, even in the face of named and ref parameters and even more crazily await expressions, even if that means Eric Lippert spends a lot of late evenings tearing his hair out :)
Somehow the code in angle brackets is missing. Here’s a solution to #4: http://ideone.com/Y65S6
This is pretty easy, except #1. I don’t know exactly what the standard says about the order of evaluation in case 2, but i think it’s undefined in both cases.
#2 has the problem that memory is leaked if the second allocation fails. Like in #1 it’s undefined which allocation comes first.
#3 is better than #2 because a unique_ptr is constructed. If the second allocation fails, the first object is deleted properly.
The solution to #4 might look like:
template
std::unique_ptr make_unique(Args&&… args)
{
return std::unique_ptr(new T(std::forward(args)…));
}