Skip to content

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.

Sign up for GitHub

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

Merged
kennykerr merged 2 commits into from
Sep 20, 2021
Merged

Improve classic COM support, and improve diagnostic for common error #1022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac5f4d7
Improve diagnostics when trying to use classic COM without <unknwn.h>
oldnewthing Sep 16, 2021
ee26c39
Merge branch 'master' into classic-com
kennykerr Sep 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 6 additions & 16 deletions strings/base_implements.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,8 @@ namespace winrt::impl
template <template <typename> typename Condition, typename T>
using tuple_if = typename tuple_if_base<Condition, T>::type;

#ifdef WINRT_IMPL_IUNKNOWN_DEFINED

template <typename T>
struct is_interface : std::disjunction<std::is_base_of<Windows::Foundation::IInspectable, T>, std::conjunction<std::is_base_of<::IUnknown, T>, std::negation<is_implements<T>>>> {};

#else

template <typename T>
struct is_interface : std::is_base_of<Windows::Foundation::IInspectable, T> {};

#endif
struct is_interface : std::disjunction<std::is_base_of<Windows::Foundation::IInspectable, T>, is_classic_com_interface<T>> {};

template <typename T>
struct is_marker : std::disjunction<std::is_base_of<marker, T>, std::is_void<T>> {};
Expand Down Expand Up @@ -485,20 +476,19 @@ namespace winrt::impl
}
};

#ifdef WINRT_IMPL_IUNKNOWN_DEFINED

template <typename D, typename I>
struct producer<D, I, std::enable_if_t<std::is_base_of_v< ::IUnknown, I> && !is_implements_v<I>>> : I
struct producer<D, I, std::enable_if_t<is_classic_com_interface<I>::value>> : I
{
#ifndef WINRT_IMPL_IUNKNOWN_DEFINED
static_assert(std::is_void_v<I> /* dependent_false */, "To implement classic COM interfaces, you must #include <unknwn.h> before including C++/WinRT headers.");
#endif
};

template <typename D, typename I>
struct producer_convert<D, I, std::enable_if_t<std::is_base_of_v< ::IUnknown, I> && !is_implements_v<I>>> : producer<D, I>
struct producer_convert<D, I, std::enable_if_t<is_classic_com_interface<I>::value>> : producer<D, I>
{
};

#endif

struct INonDelegatingInspectable : Windows::Foundation::IUnknown
{
INonDelegatingInspectable(std::nullptr_t = nullptr) noexcept {}
Expand Down
4 changes: 4 additions & 0 deletions strings/base_macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,8 @@

#ifdef __IUnknown_INTERFACE_DEFINED__
#define WINRT_IMPL_IUNKNOWN_DEFINED
#else
// Forward declare so we can talk about it.
struct IUnknown;
typedef struct _GUID GUID;
#endif
8 changes: 5 additions & 3 deletions strings/base_meta.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,14 @@ namespace winrt::impl
};

template <typename T>
#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
#ifdef __clang__
#if defined(__clang__)
#if __has_declspec_attribute(uuid)
Copy link
Collaborator

@kennykerr kennykerr Sep 20, 2021

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. 🙄

Copy link
Contributor

@sylveon sylveon Sep 20, 2021

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

Copy link
Member Author

@oldnewthing oldnewthing Sep 20, 2021

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?)

Copy link
Collaborator

@kennykerr kennykerr Sep 20, 2021

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.

Copy link
Contributor

@sylveon sylveon Sep 20, 2021

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.

Copy link
Member Author

@oldnewthing oldnewthing Sep 20, 2021 •
edited
Loading

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.

Copy link
Collaborator

@kennykerr kennykerr Sep 20, 2021

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.

Copy link
Contributor

@sylveon sylveon Sep 21, 2021 •
edited
Loading

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.

inline const guid guid_v{ __uuidof(T) };
#else
inline constexpr guid guid_v{ __uuidof(T) };
inline constexpr guid guid_v{};
#endif
#elif defined(_MSC_VER)
inline constexpr guid guid_v{ __uuidof(T) };
#else
inline constexpr guid guid_v{};
#endif
Expand Down
10 changes: 1 addition & 9 deletions strings/base_reference_produce.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,14 +212,12 @@ namespace winrt::impl
using itf = Windows::Foundation::IReference<guid>;
};

#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
template <>
struct reference_traits<GUID>
{
static auto make(GUID const& value) { return Windows::Foundation::PropertyValue::CreateGuid(value); }
static auto make(GUID const& value) { return Windows::Foundation::PropertyValue::CreateGuid(reinterpret_cast<guid const&>(value)); }
using itf = Windows::Foundation::IReference<guid>;
};
#endif

template <>
struct reference_traits<Windows::Foundation::DateTime>
Expand Down Expand Up @@ -354,14 +352,12 @@ namespace winrt::impl
using itf = Windows::Foundation::IReferenceArray<guid>;
};

#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
template <>
struct reference_traits<com_array<GUID>>
{
static auto make(array_view<GUID const> const& value) { return Windows::Foundation::PropertyValue::CreateGuidArray(reinterpret_cast<array_view<guid const> const&>(value)); }
using itf = Windows::Foundation::IReferenceArray<guid>;
};
#endif

template <>
struct reference_traits<com_array<Windows::Foundation::DateTime>>
Expand Down Expand Up @@ -444,14 +440,12 @@ namespace winrt::impl
return static_cast<T>(value.template as<Windows::Foundation::IReference<std::underlying_type_t<T>>>().Value());
}
}
#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
else if constexpr (std::is_same_v<T, com_array<GUID>>)
{
T result;
reinterpret_cast<com_array<guid>&>(result) = value.template as<typename impl::reference_traits<T>::itf>().Value();
return result;
}
#endif
else
{
return value.template as<typename impl::reference_traits<T>::itf>().Value();
Expand All @@ -473,7 +467,6 @@ namespace winrt::impl
return static_cast<T>(temp.Value());
}
}
#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
else if constexpr (std::is_same_v<T, com_array<GUID>>)
{
if (auto temp = value.template try_as<typename impl::reference_traits<T>::itf>())
Expand All @@ -483,7 +476,6 @@ namespace winrt::impl
return result;
}
}
#endif
else
{
if (auto temp = value.template try_as<typename impl::reference_traits<T>::itf>())
Expand Down
23 changes: 11 additions & 12 deletions strings/base_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,24 +141,14 @@ WINRT_EXPORT namespace winrt
{
}

#ifdef WINRT_IMPL_IUNKNOWN_DEFINED

constexpr guid(GUID const& value) noexcept :
Data1(value.Data1),
Data2(value.Data2),
Data3(value.Data3),
Data4{ value.Data4[0], value.Data4[1], value.Data4[2], value.Data4[3], value.Data4[4], value.Data4[5], value.Data4[6], value.Data4[7] }
{

}
template<bool dummy = true>
constexpr guid(GUID const& value) noexcept : guid(convert<dummy>(value)) { }

operator GUID const&() const noexcept
{
return reinterpret_cast<GUID const&>(*this);
}

#endif

constexpr explicit guid(std::string_view const value) :
guid(parse(value))
{
Expand All @@ -168,6 +158,15 @@ WINRT_EXPORT namespace winrt
guid(parse(value))
{
}

private:
template<bool, typename T>
constexpr static guid convert(T const& value) noexcept
{
return { value.Data1, value.Data2, value.Data3,
{ value.Data4[0], value.Data4[1], value.Data4[2], value.Data4[3], value.Data4[4], value.Data4[5], value.Data4[6], value.Data4[7] }
};
}
};

inline bool operator==(guid const& left, guid const& right) noexcept
Expand Down
14 changes: 4 additions & 10 deletions strings/base_windows.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,11 @@ namespace winrt::impl
return { result, take_ownership_from_abi };
}

#ifdef WINRT_IMPL_IUNKNOWN_DEFINED
template <typename T>
struct is_com_interface : std::disjunction<std::is_base_of<Windows::Foundation::IUnknown, T>, std::is_base_of<unknown_abi, T>, is_implements<T>, std::is_base_of<::IUnknown, T>> {};
#else
template<typename T>
struct is_classic_com_interface : std::conjunction<std::is_base_of<::IUnknown, T>, std::negation<is_implements<T>>> {};

template <typename T>
struct is_com_interface : std::disjunction<std::is_base_of<Windows::Foundation::IUnknown, T>, std::is_base_of<unknown_abi, T>, is_implements<T>> {};
#endif
struct is_com_interface : std::disjunction<std::is_base_of<Windows::Foundation::IUnknown, T>, std::is_base_of<unknown_abi, T>, is_implements<T>, is_classic_com_interface<T>> {};

template <typename T>
inline constexpr bool is_com_interface_v = is_com_interface<T>::value;
Expand Down Expand Up @@ -363,14 +361,10 @@ WINRT_EXPORT namespace winrt
}
}

#ifdef WINRT_IMPL_IUNKNOWN_DEFINED

inline ::IUnknown* get_unknown(Windows::Foundation::IUnknown const& object) noexcept
{
return static_cast<::IUnknown*>(get_abi(object));
}

#endif
}

WINRT_EXPORT namespace winrt::Windows::Foundation
Expand Down
29 changes: 18 additions & 11 deletions test/test/guid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
#include "pch.h"

#define STATIC_REQUIRE_GUID_EQUAL(left, right) \
STATIC_REQUIRE(left.Data1 == right.Data1); \
STATIC_REQUIRE(left.Data2 == right.Data2); \
STATIC_REQUIRE(left.Data3 == right.Data3); \
STATIC_REQUIRE(left.Data4[0] == right.Data4[0]); \
STATIC_REQUIRE(left.Data4[1] == right.Data4[1]); \
STATIC_REQUIRE(left.Data4[2] == right.Data4[2]); \
STATIC_REQUIRE(left.Data4[3] == right.Data4[3]); \
STATIC_REQUIRE(left.Data4[4] == right.Data4[4]); \
STATIC_REQUIRE(left.Data4[5] == right.Data4[5]); \
STATIC_REQUIRE(left.Data4[6] == right.Data4[6]); \
STATIC_REQUIRE(left.Data4[7] == right.Data4[7]);

TEST_CASE("guid")
{
constexpr winrt::guid expected{ 0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff } };

STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data1 == expected.Data1);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data2 == expected.Data2);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data3 == expected.Data3);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[0] == expected.Data4[0]);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[1] == expected.Data4[1]);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[2] == expected.Data4[2]);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[3] == expected.Data4[3]);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[4] == expected.Data4[4]);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[5] == expected.Data4[5]);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[6] == expected.Data4[6]);
STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[7] == expected.Data4[7]);
STATIC_REQUIRE_GUID_EQUAL(winrt::guid("00112233-4455-6677-8899-aabbccddeeff"), expected);

REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff") == expected);
REQUIRE(winrt::guid({ "{00112233-4455-6677-8899-aabbccddeeff}" + 1, 36 }) == expected);
Expand All @@ -26,4 +29,8 @@ TEST_CASE("guid")
REQUIRE_THROWS_AS(winrt::guid("00112233-4455-6677-8899-aabbccddeeff with extra"), std::invalid_argument);
REQUIRE_THROWS_AS(winrt::guid("xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"), std::invalid_argument);
REQUIRE_THROWS_AS(winrt::guid("{00112233-4455-6677-8899-aabbccddeeff}"), std::invalid_argument);

// Verify that you can constexpr-construct a guid from a GUID.
constexpr winrt::guid from_abi_guid = GUID{ 0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff } };
STATIC_REQUIRE_GUID_EQUAL(from_abi_guid, expected);
}
1 change: 1 addition & 0 deletions test/test_component_derived/pch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
#pragma once
#include <unknwn.h> // Nested.HierarchyD uses classic COM interface IReferenceTrackerExtension