value.set(T&&) is problematic #147
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The rvalue reference conditional was implemented with a universal/forwarding reference. This has problems: Updated tests to do additional checks. Added calls to set() with an rvalue and (nonconst) lvalue version of 'cmp', which caused the following link errors in the original picojson code: undefined reference to `void picojson::value::set<bool>(bool&&)' undefined reference to `void picojson::value::set<bool&>(bool&)' undefined reference to `void picojson::value::set<bool>(bool&&)' undefined reference to `void picojson::value::set<bool&>(bool&)' undefined reference to `void picojson::value::set<double>(double&&)' undefined reference to `void picojson::value::set<double&>(double&)' undefined reference to `void picojson::value::set<double>(double&&)' undefined reference to `void picojson::value::set<double&>(double&)' undefined reference to `void picojson::value::set<std::string&>(std::string&)' Afterwards all tests pass. The problem is the definition of the template `set(T&&)` function, which (due to being a forwarding reference) is a better match than the set(T const&) versions when passing an rvalue or a non-const lvalue (since it can be an exact match, while the T const& version must undero a const-conversion). The change in picojson is to remove the set(T&&) template and provide the 3 explicit overloads for the rvalue-reference types intended (where moving makes sense). Now it stops matching the other types that were not intended.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an issue where we cannot call
myValue.set(1.5)ordouble d=1.5; myValue.set(d);because both versions fail to link. They both match the forwarding reference set(T&&) function better than the expectedset(T const&)version, and it is not defined for several types. Problems occur withdouble,bool,int64, andstd::string.The problem with std::string is only with lvalues, in that the "non-const to const" conversion makes theset(std::string const&)a worse match than the rvalue forwardig reference, which was not defined.I changed the forwarding reference from a template to three explicit function declarations, so we don't have such a large set of interfaces inadvertently caught by the template.
I also ran clang-format in another commit, prior to making the change, since the code did not match its own .clang-format file format.
The changes in the tests will show the link failures mentioned in the commit message, if run with the original picojson header, but it passes after my changes.
The issue with std::string& is that an lvalue