Prevent inadvertent assignment to temporary object - #825
Merged
Kenny Kerr (kennykerr) merged 1 commit intoDec 17, 2020
Conversation
Common porting error coming from languages that have properties is trying to set a C++/WinRT property by doing ```cpp o.Property() = value; ``` due to overzealous application of the rule "In C++/WinRT, you access a property by doing `o.Property()`." Fix this by making all assignment operators require the assigned-to variable to be an lvalue, rendering the above mistake a compile-time error. Assigning to an rvalue is not interesting because the rvalue has no name, so you have no way of using the assigned-to object. There are some wacky fringe cases where you might be doing this on purpose: ```cpp get_abi(hstring() = L"Hello") ``` gives you an ABI string handle for a temporary. But this is decidedly non-idiomatic and I think it's okay to break them. The idiomatic way of writing this is simply ```cpp get_abi(hstring(L"Hello")) ``` Assignment operators default to non-ref-qualified, which makes them applicable to both lvalue and rvalue references. We must explicitly redeclare them as lvalue ref-qualified in order to remove the rvalue ref-qualified version. Once you have an explicit assignment operator, the implicit copy and move constructors disappear, so we have to bring them back explicitly. Once you add explicit copy and move constructors, then the implicit default constructor disappears. Fortunately, we never relied upon the implicit default constructor, so we don't have to to make explicit versions.
Chris Guzak (ChrisGuzak)
approved these changes
Dec 16, 2020
Collaborator
|
Ryan Shepherd (@DefaultRyan) see any issues with this? It looks good to me. |
Ryan Shepherd (DefaultRyan)
approved these changes
Dec 17, 2020
Member
Looks good to me. Approved. |
This file contains hidden or 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
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.
Common porting error coming from languages that have properties as native concepts is trying to set a C++/WinRT property by doing
due to overzealous application of the rule "In C++/WinRT, you access a property by doing
o.Property()."Fix this by making all assignment operators require the assigned-to variable to be an lvalue, rendering the above mistake a compile-time error.
Assigning to an rvalue is not interesting because the rvalue has no name, so you have no way of using the assigned-to object. There are some wacky fringe cases where you might be doing this on purpose:
get_abi(hstring() = L"Hello")gives you an ABI string handle for a temporary. But this is decidedly non-idiomatic and I'm fairly confident nobody does this on purpose. The idiomatic way of writing this is simply
This fixes issue #359
Implementation notes
Assignment operators default to non-ref-qualified, which makes them applicable to both lvalue and rvalue references. We must explicitly redeclare them as lvalue ref-qualified in order to remove the rvalue ref-qualified version.
Once you have an explicit assignment operator, the implicit copy and move constructors disappear, so we have to bring them back explicitly.
Once you add explicit copy and move constructors, then the implicit default constructor disappears. Fortunately, we never relied upon the implicit default constructor, so we don't have to to make explicit versions.