This is an archive of the discontinued LLVM Phabricator instance.

Fix warnings about pessimizing return moves for C++11 and higher
ClosedPublic

Authored by dim on Jul 21 2015, 12:19 PM.

Details

Summary

Throughout the libc++ headers, there are a few instances where
_VSTD::move() is used to return a local variable. Howard commented in
r189039 that these were there "for non-obvious reasons such as to help
things limp along in C++03 language mode".

However, when compiling these headers with warnings on, and in C++11 or
higher mode (like we do in FreeBSD), they cause the following complaints
about pessimizing moves:

In file included from tests.cpp:26:
In file included from tests.hpp:29:
/usr/include/c++/v1/map:1368:12: error: moving a local object in a return statement prevents copy elision [-Werror,-Wpessimizing-move]
    return _VSTD::move(__h);  // explicitly moved for C++03
           ^
/usr/include/c++/v1/__config:368:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_NAMESPACE
              ^

Attempt to fix this by adding a _LIBCPP_EXPLICIT_MOVE() macro to
__config, which gets defined to _VSTD::move for pre-C++11, and to
nothing for C++11 and later.

I am not completely satisfied with the macro name (I also considered
_LIBCPP_COMPAT_MOVE and some other variants), so suggestions are
welcome. :)

Diff Detail

Event Timeline

dim updated this revision to Diff 30278.Jul 21 2015, 12:19 PM
dim retitled this revision from to Fix warnings about pessimizing return moves for C++11 and higher.
dim updated this object.
dim added reviewers: mclow.lists, EricWF, howard.hinnant.
dim added a subscriber: cfe-commits.

From the peanut gallery: Could someone explain why this warning is actually
a false positive in C++03 mode? It seems to me that the std::move there
disables NRVO in C++03 to exactly the same degree as it disables NRVO in
C++11. Admittedly I have no idea what _VSTD::move(x) does to x in C++03
mode; I'd have expected it to be a no-op, except for disabling RVO.

–Arthur

EricWF edited edge metadata.Jul 21 2015, 5:29 PM

Thanks for the patch. I ran into this issue the other day and I'm glad to see it fixed.

A little rational: The explicit move's are needed in order to "move" a unique_ptr in C++03. There is a special definition of std::move in memory at line 3100 that performs some hacks to make unique_ptr movable. I don't think any other classes benefit from the "explicit move" in C++03.

I am not completely satisfied with the macro name (I also considered
_LIBCPP_COMPAT_MOVE and some other variants), so suggestions are
welcome. :)

I think the name is fine and there is no need to change it. However maybe the name should reflect the fact that it is only needed for unique_ptr? Something like _LIBCPP_UNIQUE_PTR_MOVE would better convey the weirdness this macro is doing.

include/__config
719

We need the explicit move whenever _LIBCPP_HAS_NO_RVALUE_REFERENCES is defined. Please use that instead.

include/algorithm
854

This one seems suspect. I don't really know if we need the move here. Do you know why this here?

dim updated this revision to Diff 30324.Jul 22 2015, 12:07 AM
dim edited edge metadata.

Used _LIBCPP_HAS_NO_RVALUE_REFERENCES instead of checking __cplusplus.

mclow.lists added inline comments.Jul 22 2015, 8:42 AM
include/__config
724

I think that this needs more exploration; someone needs to look at the codegen.

return x;

is different from:

return (x);   // this is an expression

I suspect that only one will trigger RVO.

dim added inline comments.Jul 25 2015, 6:57 AM
include/__config
724

Would you really think so? I would think the parentheses are superfluous, and can be removed by parsing stages before it gets to RVO. But I will try to check the codegen anyway.

Alternatively, since the macro is for 'internal use only', we might try to just remove the parentheses, e.g.:

#ifdef _LIBCPP_HAS_NO_RVALUE_REFERENCES
#  define _LIBCPP_EXPLICIT_MOVE(x) _VSTD::move(x)
#else
#  define _LIBCPP_EXPLICIT_MOVE(x) x
#endif

It is a bit ugly, but at least the code itself does not have to be littered with #ifdefs.

EricWF accepted this revision.Jul 30 2015, 7:13 PM
EricWF edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jul 30 2015, 7:13 PM

@dim Has this been committed or is there something holding you back?

dim added a comment.Aug 18 2015, 11:41 PM

@EricWF, no, this just dropped off my radar, sorry. I will commit it now. :)

dim closed this revision.Aug 18 2015, 11:44 PM