Optimize C++/WinRT implementation signatures#512
Merged
Conversation
BenJKuhn
reviewed
Jul 11, 2019
|
|
||
| bool is_async() const | ||
| { | ||
| if (is_put_overload(m_method)) |
Contributor
There was a problem hiding this comment.
per the detailed description above, this function's body is optimized for actual usage in a specific case, rather than simply providing an answer to whether the method is async. It would make sense to pick a more specific name and/or add a comment that's a bit more durable. Otherwise it's very non-obvious as to why a property setter is considered "async".
Contributor
Author
There was a problem hiding this comment.
Sure, I'll add some of that explanation as a comment.
Contributor
Author
There was a problem hiding this comment.
Done - added a comment.
BenJKuhn
approved these changes
Jul 11, 2019
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.
The default implementation signatures generated by cppwinrt for a component weren't always ideal. Specifically, properties always took their single argument by value thus forcing a copy. Developers can easily fix this in their implementation but many not think to do so. This update fixes that, but I'll add a bit of an explanation for the internal logic in case it's not that obvious.
The
is_asynchelper decides whether a given method (or property) is async. Primarily this is about detecting whether the method returns one of the WinRT async interfaces, but it is also used under certain conditions for properties, which are not async in that sense. The reason for the former is obvious, but for the latter it can be a little subtle.WinRT parameter passing conventions include the notion that input parameters of collection types may be read or copied but should not be stored directly since this would lead to instability as the collection is shared by the caller and callee. The exception to this rule is property setters where the callee may simply store a reference to the collection. The collection thus becomes async in the sense that it is expected to remain valid beyond the duration of the call.
Coming back to C++/WinRT, the language projection optimizes for various input parameter bindings. This allows for things like efficiently calling a WinRT method expecting an
IVector<int>with a localstd::vector<int>. This is possible because the implementation ofIVector<int>produced in these cases has special invalidation logic that will prevent theIVector<int>from being used beyond the duration of the call. Of course, if the method is known to be async then this logic should not be provided and the caller should instead be required to provide an implementation that is not locally bound. All of this is handled automatically by C++/WinRT under the hood and relies on thisis_asyncfunction to determine when to apply this policy.The update here simply ensures that this policy of treating properties as async is not applied to an implementation since the optimization only affects the call site. The callee doesn't know anything about how the caller created the argument and can simply following the normal rules for lifetime.
The implementation continues to treat all async methods the same way to ensure that parameters are passed by value under the expectation that they will be implemented as coroutines.