Skip to content

feat: add make_array_of_(uuid|guid) #539

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

Open
roxk wants to merge 10 commits into
base: master
Choose a base branch
from
+112 −46
Open

feat: add make_array_of_(uuid|guid) #539

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2e5346b
COM server refactor and improvements
sylveon Jul 24, 2025
960310f
Format + remove mutable
sylveon Jul 24, 2025
e363b7c
Address review comments
sylveon Jul 24, 2025
bd7d9a6
Formatting, again
sylveon Jul 24, 2025
e080b13
Fix clang crash
sylveon Jul 24, 2025
b41ab25
Fix __has_cpp_attribute check
sylveon Jul 29, 2025
c758246
feat: add make_array_of_(uuid|guid)
roxk Aug 1, 2025
229a503
Merge branch 'master' into specify-class-once
dunhor Aug 6, 2025
16a2471
fix: keep the original array overload
roxk Aug 10, 2025
04511fe
Merge branch 'specify-class-once' of https://github.com/roxk/wil into…
roxk Aug 10, 2025
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
41 changes: 39 additions & 2 deletions include/wil/cppwinrt_authoring.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,16 +172,22 @@ unique_com_class_object_cookie
return registration;
}

template <typename... Types>
struct clsid_array
{
std::array<GUID, sizeof...(Types)> value;
};

template <typename... Ts>
WI_NODISCARD_REASON("The classes are unregistered when the returned value is destructed")
std::vector<unique_com_class_object_cookie> register_com_server(
std::array<GUID, sizeof...(Ts)> const& guids, DWORD context = CLSCTX_LOCAL_SERVER, DWORD flags = REGCLS_MULTIPLEUSE)
wil::clsid_array<Ts...> const& clsids, DWORD context = CLSCTX_LOCAL_SERVER, DWORD flags = REGCLS_MULTIPLEUSE)
Copy link
Copy Markdown
Contributor

@sylveon sylveon Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend keeping the std::array parameter and making clsid_array a new overload.

The overload can simply pass it on to the std::array version

Copy link
Copy Markdown
Contributor Author

@roxk roxk Aug 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind that, but let's see what @dunhor vote for.

Copy link
Copy Markdown
Member

@dunhor dunhor Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd tend to agree with @sylveon here, but I'll take it one step further - I don't really see a value for the clsid_array type at all. In fact, I'd say that it harms the API overall. Consider the way that most people will use this funciton:

std::array<GUID> clsids = { CLSID_Foo, CLSID_Bar };
auto unregister = wil::register_com_server<Foo, Bar>(clsids);

Tying the type names of the GUIDs to the array parameter is actively harmful and violates your "don't repeat twice" principle (I'd need to provide the type names in the type of clsid_array in this case).

I'd say remove this type altogether. A type alias might be fine, but still probably unnecessary.

Copy link
Copy Markdown
Contributor

@sylveon sylveon Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit is when you are using __uuidof or winrt::guid_of, tying the types to the array allows template type deduction to work, preventing you from typing the name twice.

Like this:

wil::register_com_server(wil::make_array_of_uuid<Foo, Bar>());

But we definitely need to keep the untyped array overloads for examples like the one you have.

Copy link
Copy Markdown
Member

@dunhor dunhor Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I see.

Copy link
Copy Markdown
Member

@dunhor dunhor Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, you state that you are getting ambiguous overload errors. I'm wondering if that's because the tests are calling using double braces ({{ CLSID_A, CLSID_B, ... }}). If you switch the tests over to single braces, I wonder if that resolves the errors.

Copy link
Copy Markdown
Contributor

@sylveon sylveon Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double braces are required for std::array. We would've needed std::initializer_list for single braces.

Copy link
Copy Markdown
Member

@dunhor dunhor Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::array supports aggregate initialization, so single braces should be fine https://godbolt.org/z/o5cPPjYKx

Copy link
Copy Markdown
Contributor

@sylveon sylveon Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, I could swear that wasn't the case in specific instances. Guess I was wrong.

Copy link
Copy Markdown
Member

@dunhor dunhor Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: https://en.cppreference.com/w/cpp/language/aggregate_initialization.html#Appertainment

Specifically:

Otherwise, elem is an aggregate and that subaggregate is replaced in the list of aggregate elements by the sequence of its own aggregate elements, and the appertainment analysis resumes with the first such element and the same initializer clause. In other words, these rules apply recursively to the aggregate’s subaggregates.

Which probably makes my previous statement about ambiguous overloads incorrect, at least as long as clsid_array is an aggregate. I.e. it would need a constructor, however that constructor would not need to be explicit (though, frankly, it's probably good to be explicit anyway)

{
std::vector<wil::unique_com_class_object_cookie> registrations;
registrations.reserve(sizeof...(Ts));

std::size_t i = 0;
(registrations.push_back(wil::register_com_server<Ts>(guids[i++], context, flags | REGCLS_SUSPENDED)), ...);
(registrations.push_back(wil::register_com_server<Ts>(clsids.value[i++], context, flags | REGCLS_SUSPENDED)), ...);

// allow the user to keep class objects suspended if they've explicitly passed REGCLS_SUSPENDED.
if (!WI_IsFlagSet(flags, REGCLS_SUSPENDED))
Expand All @@ -192,6 +198,37 @@ std::vector<unique_com_class_object_cookie> register_com_server(
return registrations;
}

namespace details
{
template <typename T>
struct has_iid
{
template <typename U = T, std::enable_if_t<std::is_same_v<GUID, std::decay_t<decltype(__uuidof(U))>>, int> = 0>
static std::true_type invoke(int);

template <typename U = T>
static std::false_type invoke(float);

static constexpr bool value = decltype(invoke(0))::value;
};

template <typename T>
constexpr bool has_iid_v = has_iid<T>::value;
}

template <typename... Types>
constexpr auto make_array_of_uuid()
Copy link
Copy Markdown
Member

@dunhor dunhor Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like the names make_clsid_array_uuid and make_clsid_array_guid (or perhaps make_clsid_array_uuidof and make_clsid_array_guid_of to make it more explicit which operation is being used to form the CLSIDs). This makes it clearer what the function's purpose is. What do others think here?

Copy link
Copy Markdown
Member

@dunhor dunhor Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that the __uuidof version could be useful outside of the scope of CLSIDs, e.g. if you were to implement something like RuntimeClass and wanted an array of IIDs when implementing QueryInterface etc.

Copy link
Copy Markdown
Contributor

@sylveon sylveon Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about make_clsid_array and make_clsid_array_winrt? I don't particularly like uuid vs guid because it is not clear what are the differences.

Copy link
Copy Markdown
Contributor Author

@roxk roxk Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about make_clsid_array and make_clsid_array_winrt? I don't particularly like uuid vs guid because it is not clear what are the differences.

Seems dunhor's suggestion can answer the question? I'd expect user typing make_clsid_array_ and then seeing uuidof vs guid_of would be able to tell which is which

Copy link
Copy Markdown
Member

@ChrisGuzak ChrisGuzak Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guid/uuid are the most generic. clsid and iid are specializations.
it is odd that clsid_array is the building block. I'd expect guid/uuid to be at the bottom.

these are all aliases for the same thing, so using different names can be dangerous due to the false sense of type safety.

{
return clsid_array<Types...>{std::array<GUID, sizeof...(Types)>{__uuidof(Types)...}};
}

template <typename... Types>
constexpr auto make_array_of_guid()
{
static_assert(!(... && details::has_iid_v<Types>), "Use make_array_of_uuid for COM class instead");
return clsid_array<Types...>{std::array<GUID, sizeof...(Types)>{winrt::guid_of<Types>()...}};
}

#endif // (!defined(__WIL_CPPWINRT_AUTHORING_INCLUDED_COM_SERVER) && defined(__WIL_CPPWINRT_AUTHORING_INCLUDED_ICLASSFACTORY) && defined(_COMBASEAPI_H_))

#if (!defined(__WIL_CPPWINRT_AUTHORING_INCLUDED_FOUNDATION) && defined(WINRT_Windows_Foundation_H)) || \
Expand Down
18 changes: 18 additions & 0 deletions tests/CppWinRTAuthoringTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,24 @@ TEST_CASE("CppWinRTAuthoringTests::MultiRegisterComServer", "[com_server]")
}
}

TEST_CASE("CppWinRTAuthoringTests::MultiRegisterComServerWithMakeArrayOfUuid", "[com_server]")
{
auto revoker = wil::register_com_server(wil::make_array_of_uuid<MyServer, BuggyServer>());

auto instance = create_server_instance(__uuidof(MyServer));
REQUIRE(winrt::get_module_lock() == 1);

try
{
auto instance2 = create_server_instance(__uuidof(BuggyServer));
REQUIRE(false);
}
catch (winrt::hresult_error const& e)
{
REQUIRE(e.code() == E_ACCESSDENIED);
}
}

TEST_CASE("CppWinRTAuthoringTests::MultiRegisterComServerUnregistersOnFail", "[com_server]")
{
winrt::init_apartment();
Expand Down