-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support moving from rvalues in std::initializer_list #663
Conversation
This commit works around an issue in std::initializer_list design. By using a detail::json_ref proxy with a mutable value inside, rvalue-ness of an input to list initializer is remembered and used later to move from the proxy instead of copying.
std::initializer_list does not own the temporaries created in its initialization. Therefore, storing it in an independent stack variable is unsafe.
C++ overload resolution/list initialization rules are hard.
On MSVC compiler, temporaries that are constructed during a list initialization, are sometimes destroyed even before calling the initializing constructor, instead of at the end of the containing full-expression. This is clearly non-conforming to [class.temporary]. As the impact of this bug is silently producing incorrect JSON values, move eagerly from rvalues to be safe. See https://stackoverflow.com/questions/24586411
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks a lot! |
I have some conversion warnings when compiling with GCC with maximal warnings:
Any ideas on this? |
Furthermore, there is a Weffc++ warning that |
As for the conversion warnings, it seems to be an unwanted (but harmless) interplay with As for constructors, json_ref should only be movable, and default move constructor is fine on a conforming C++11 compiler. So, adding |
This should fix the underlying problem of inefficient list initialization, which has led me to filing #657.
By using a rvalue-aware proxy type inside all uses of
std::initializer_list
, simple, obvious usages such asshould just work without calling
json
copy constructor even once.Possible downsides for this solution:
std::initializer_list
values, which is not common in C++. All normal usage should remain unaffected (just a bit faster).