-
Notifications
You must be signed in to change notification settings - Fork 264
Add value type support to as() and try_as() #695
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
Conversation
This permits fluent usage, which permits left-to-right reading instead of prefix usage which forces the eyes to dart back and forth. Example of using `as` instead of `unbox_value`: ```cpp // old and busted with unbox_value() auto h = unbox_value<Size>(SelectedItem().Tag()).Height; auto h = unbox_value<Size>(propertySet.Lookup(L"key")).Height; // new hotness with as() auto h = SelectedItem().Tag().as<Size>().Height; auto h = propertySet.Lookup(L"key").as<Size>().Height; ``` Example of using `try_as` instead of `unbox_value_or`: ```cpp // old and busted with unbox_value_or() auto h = unbox_value_or<Size>(SelectedItem().Tag(), default_size).Height; auto h = unbox_value_or<Size>(propertySet.TryLookup(L"key"), default_size).Height; // new hotness with try_as() + std::optional auto h = SelectedItem().Tag().try_as<Size>().value_or(default_size).Height; auto h = propertySet.TryLookup(L"key").try_as<Size>().value_or(default_size).Height; ``` Including `winrt/Windows.Foundation.h` lights up these overloads. If you don't include it, then you get a compile-time error that points you to a line with the comment ```cpp // You must include <winrt/Windows.Foundation.h> to use this overload. ``` The new functionality is overloaded onto the existing names `as` and `try_as` to avoid introducing new names into `com_ptr` and `IUnknown`. Factored unit tests to make it easier to add tests for the new overload. Also added a new unit test for `com_ptr` duck typing, which is an interesting corner case: You can use `com_ptr` for things that aren't actually COM objects, as long as they quack like a COM object. Care must be taken not to cast such objects to IUnknown, because the decoys won't have the same vtable layout (or possibly no vtable at all). Note such care in the new `impl::try_as` method, where we cast the raw pointer to `com_ptr` if it isn't a COM interface, thereby allowing the duck typing to proceed. The trickiest parts are finding the right definition for the `is_com_interface` detector class and deploying it correctly via `enable_if_t` and `if constexpr`. The `try_as` overload which sets an output parameter in particular needs to detect `is_com_interface` + `com_ptr`. I reused the `wrapped_type` detector class for detecting `com_ptr`. Also deleted apparently-unused `is_detected` detector class.
|
Alternative to #688 |
| } | ||
| } | ||
| #ifdef WINRT_IMPL_IUNKNOWN_DEFINED | ||
| else if constexpr (std::is_same_v<T, GUID>) |
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.
Are shouty-GUID and guid the same type?
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.
Sadly no. Shouty-GUID is the Win32 guid. Lowercase guid is the Windows Runtime Guid.
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.
They couldn't be the same type because GUID is defined repeatedly as a macro in different places and that breaks C++ modules.
| } | ||
|
|
||
| template <typename T, typename Ret = T, typename From, typename U> | ||
| Ret unbox_value_type_or(From&& value, U&& default_value) |
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.
What do you think about a defaulted version for default_value, so someone can say unbox_value_type_or_default<int32_t> directly, instead of passing in the default value?
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.
All WinRT types have an "all zero bytes" representation for their default values, but I'm not sure about adding another function for this though. How common is it to want that default when that default isn't normally a usable value? And you can obviously always request that as the default.
| { | ||
| to = try_as<impl::wrapped_type_t<To>>(); | ||
| return static_cast<bool>(to); | ||
| if constexpr (impl::is_com_interface_v<To> || !std::is_same_v<To, impl::wrapped_type_t<To>>) |
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.
The tricky part is these enable_if_t's and if constexpr's. I'm still not confident that I got them all right.
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.
Exhaustive testing of edge cases is the only way I feel confident about this stuff.
kennykerr
left a comment
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 great!
This permits fluent usage, which lets you read the code left-to-right instead of prefix usage which forces the eyes to dart back and forth.
Example of using
asinstead ofunbox_value:Example of using
try_asinstead ofunbox_value_or:Including
winrt/Windows.Foundation.hlights up these overloads. If you don't include it, then you get a compile-time error that points you to a line with the comment// You must include <winrt/Windows.Foundation.h> to use this overload.The new functionality is overloaded onto the existing names
asandtry_asto avoid introducing new names intocom_ptrandIUnknown.Factored unit tests to make it easier to add tests for the new overload. Also added a new unit test for
com_ptrduck typing, which is an interesting corner case: You can usecom_ptrfor things that aren't actually COM objects, as long as they quack like a COM object. Care must be taken not to cast such objects to IUnknown, because the decoys won't have the same vtable layout (or possibly no vtable at all). Note such care in the newimpl::try_asmethod, where we cast the raw pointer tocom_ptrif it isn't a COM interface, thereby allowing the duck typing to proceed.The trickiest parts are finding the right definition for the
is_com_interfacedetector class and deploying it correctly viaenable_if_tandif constexpr. Thetry_asoverload which sets an output parameter in particular needs to detectis_com_interface+com_ptr. I reused thewrapped_typedetector class for detectingcom_ptr.Also deleted apparently-unused
is_detecteddetector class.