-
Notifications
You must be signed in to change notification settings - Fork 263
Improve classic COM support, and improve diagnostic for common error #1022
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
Before this change, if you tried to use classic COM without
first including <unknwn.h>, C++/WinRT spit out really confusing
error messages like
```cpp
auto widget = object.as<::IClassicComInterface>();
```
> error: 'unbox_value_type': no matching overloaded function found
or worse: Just ignored you!
```cpp
struct Widget : WidgetT<Widget, ::IPersistStream>
{
// IPersistStream is simply ignored!
};
```
This change relaxes the rules around `<unknwn.h>` and improves
diagnostics.
1. If you want to use `com_ptr<I>`, `as<I>`, etc.
with classic COM interfaces, you need only include `<unknwn.h>`
at *some point* before using them. Doesn't have to be included
before `winrt/base.h`. (And really, since you need to include
`<unknwn.h>` in order to define *any* classic COM interfaces,
the prerquisite is automatically satisfied in practice.)
2. If you want to use `implements<...>` with classic COM interfaces,
you stlil need to include `<unknwn.h>` before `winrt/base.h`,
but we improved the diagnostics so that if you forget, you get
a compiler error instead of a runtime error:
> To implement classic COM interfaces, you must `#include <unknwn.h>` before including C++/WinRT headers.
Implementation notes
====================
The key step is forward-declaring the missing types
`IUnknown` and `GUID` so that we can talk *about* them
without needing to know what they are. This trick allows us to
remove a large number of `#ifdef WINRT_IUNKNOWN_DEFINED`
tests, which unlocks 80% of this feature.
To avoid repetition, introduced a new type trait
`is_classic_com_interface<T>`, which factors out many
uses of `is_base_of<::IUnknown, T> && !is_implements<T>`.
Having a forward declaration of `IUnknown` lets us detect
that a type is a classic COM interface by seeing if it
derives from `::IUnknown`.
Having a forward declaration of `GUID` lets us reinterpret-cast
them to `guid`, which covers nearly everything.
The only place where we really need to know what's in a `GUID`
is the `constexpr guid(GUID const&)` constructor. For that,
the constructor is templated with a dummy default parameter.
The dummy default parameter delays expansion until the
constructor is invoked. We then forward the `GUID` to
a templated helper as a dependent type `T`, so the compiler
accepts the helper despite not knowing what T's members are.
This change found a bug in the unit tests, because Nested.HierarchyD used
classic COM interface `IReferenceTrackerExtension` without having
first included `unknwn.h`, so it was ignored!
(Amusing note: The implementation of this feature got shorter
the longer I worked on it, as I gradually realized that nearly all
of the places we needed to protect the use of `GUID` didn't need
the duck-typing trick employed in the `guid(GUID const&)` constructor.)
| #ifdef WINRT_IMPL_IUNKNOWN_DEFINED | ||
| #ifdef __clang__ | ||
| #if defined(__clang__) | ||
| #if __has_declspec_attribute(uuid) |
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.
Can't believe Clang still doesn't treat __uuidof as a constexpr expression. 🙄
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.
It should, I reported a bug back in 2018 and that bug was fixed in 2020: https://bugs.llvm.org/show_bug.cgi?id=38490
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.
Cool! Now to figure out how to detect this bugfix at compile time. Any clues? (Or is it okay to require everyone to have a fixed clang?)
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.
Assuming this fix has been published, I would be ok with just requiring it. Older compilers can just stick with an older version of C++/WinRT.
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.
We can version detect or use something to check if an expression is constexpr, but Clang 11 is almost 1 year old so I'm not sure how much it's worth to support older. VS seems to require (or strongly suggest) an up to date clang to be able to use the LLVM platform toolset so I imagine the amount of consumers of cppwinrt with an out of date clang to be somewhat low.
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, godbolt's clang hasn't been updated with the fix. Also sadly, the compile-time detection lets you change code flow based on whether something is constexpr-evaluatable, but I don't see how you can use it to change whether a variable is constexpr.
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.
Too bad - looks like its not been fixed after all.
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.
This is weird, it works on my local copy of Clang 12 (we get to the link stage, and that fails cause I didn't add a main):
T:\Projects>type constexpr.cpp
typedef struct _GUID {
unsigned long Data1;
unsigned short Data2;
unsigned short Data3;
unsigned char Data4[ 8 ];
} GUID;
struct __declspec(uuid("67f4c08e-703b-4afa-bb75-d13a3ba2c583")) blarg;
constexpr GUID foo = __uuidof(blarg);
T:\Projects>clang constexpr.cpp
LINK : fatal error LNK1561: entry point must be defined
clang: error: linker command failed with exit code 1561 (use -v to see invocation)
T:\Projects>clang --version
clang version 12.0.0
Target: i686-pc-windows-msvc
Thread model: posix
InstalledDir: C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\bin
The version of clang on Godbolt runs on Linux - this might be related.
Before this change, if you tried to use classic COM without first including <unknwn.h>, C++/WinRT spit out really confusing error messages like
auto widget = object.as<::IClassicComInterface>();or worse: Just ignored you!
This change relaxes the rules around
<unknwn.h>and improves diagnostics.If you want to use
com_ptr<I>,as<I>, etc. with classic COM interfaces, you need only include<unknwn.h>at some point before using them. Doesn't have to be included beforewinrt/base.h. (And really, since you need to include<unknwn.h>in order to define any classic COM interfaces, the prerequisite is automatically satisfied in practice.)If you want to use
implements<...>with classic COM interfaces, you still need to include<unknwn.h>beforewinrt/base.h, but we improved the diagnostics so that if you forget, you get a compiler error instead of a runtime error:Implementation notes
The key step is forward-declaring the missing types
IUnknownandGUIDso that we can talk about them without needing to know what they are. This trick allows us to remove a large number of#ifdef WINRT_IUNKNOWN_DEFINEDtests, which unlocks 80% of this feature.To avoid repetition, introduced a new type trait
is_classic_com_interface<T>, which factors out many uses ofis_base_of<::IUnknown, T> && !is_implements<T>.Having a forward declaration of
IUnknownlets us detect that a type is a classic COM interface by seeing if it derives from::IUnknown.Having a forward declaration of
GUIDlets us reinterpret-cast them toguid, which covers nearly everythingGUID-related.The only place where we really need to know what's in a
GUIDis theconstexpr guid(GUID const&)constructor. For that, the constructor is templated with a dummy default parameter. The dummy default parameter delays expansion until the constructor is invoked. We then forward theGUIDto a templated helper as a dependent typeT, so the compiler accepts the helper despite not knowing whatT's members are.This change found a bug in the unit tests, because Nested.HierarchyD used classic COM interface
IReferenceTrackerExtensionwithout having first includedunknwn.h, so it was ignored!(Amusing note: The implementation of this feature got shorter the longer I worked on it, as I gradually realized that nearly all of the places we needed to protect the use of
GUIDdidn't need the duck-typing trick employed in theguid(GUID const&)constructor. It also took me a while to find a short-and-sweet version of the duck-typing trick. My earlier attempts were overly complicated and required new unit tests because I was changing the constructors too much. But the short version doesn't change any constructor overload matching at all, which means that we don't need fancy new tests either. Just a quick check thatconstexprconstruction still works.)