From 22b8c72924762cb8886f0eac00a7f9fbacdb8c40 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Tue, 14 Feb 2023 09:09:00 -0600 Subject: [PATCH 1/4] Remove unnecessary const qualifier on conversion returns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The conversion from `producer_convert` to `const producer_ref` does not improve const-correctness, and in compliant compilers¹ prevents the conversion of `producer_convert` (ala `winrt::implements`) to `producer_ref`'s base type `T`. In brief², this is because conversion acts as rvalue reference construction via `T(T&&)`; a `const`-qualified rvalue cannot bind an rvalue reference. The `const`ness here doesn't impart any additional safety, either: - It contains only an untyped pointer, the constness of which does not restrict its holder from any additional access to either the implementation or projected types. - It exists to convert to its base type T, and it will do so by copy. That copy will contain (hidden) the same pointer, but will often not be `const`-qualified; any code that holds it will likely have unfettered access to that In short, it's a `const`-qualified temporary that exists to convert a `winrt::implements` from non-`const` `D` to non-`const` `I`. Given that holding on to a `producer_ref` is in violation of the C++/WinRT `impl` namespace contract _anyway_, be it `const`-qualified or not, this should be a safe change to make. Closes #1273 ¹ http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2077 ² https://stackoverflow.com/a/70654432 --- strings/base_implements.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/strings/base_implements.h b/strings/base_implements.h index e545b2a57..e40b1669b 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -110,12 +110,12 @@ namespace winrt::impl template struct producer_convert : producer::type> { - operator producer_ref const() const noexcept + operator producer_ref() const noexcept { return { (produce::type>*)this }; } - operator producer_vtable const() const noexcept + operator producer_vtable() const noexcept { return { (void*)this }; } From b48a2f3e36a9881453ac76d5ab54ba916012b3c6 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Wed, 22 Feb 2023 19:31:42 -0600 Subject: [PATCH 2/4] Switch to a clang-only operator I --- strings/base_implements.h | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/strings/base_implements.h b/strings/base_implements.h index e40b1669b..ca69bc38b 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -110,12 +110,27 @@ namespace winrt::impl template struct producer_convert : producer::type> { - operator producer_ref() const noexcept +#ifdef __clang__ + // This is sub-optimal in that it requires an AddRef and Release of the + // implementation type for every conversion, but it works around an + // issue where Clang ignores the conversion of producer_ref const + // to I&& (an rvalue ref that cannot bind a const rvalue). + // See CWG rev. 110 active issue 2077, "Overload resolution and invalid + // rvalue-reference initialization" + operator I() const noexcept + { + I result{ nullptr }; + copy_from_abi(result, (produce::type>*)this); + return result; + } +#else + operator producer_ref const() const noexcept { return { (produce::type>*)this }; } +#endif - operator producer_vtable() const noexcept + operator producer_vtable const() const noexcept { return { (void*)this }; } From 7e69a1aac8610c1c5006ec67f42bd69ebae66daf Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Thu, 23 Feb 2023 12:53:20 -0600 Subject: [PATCH 3/4] Add a uniform initialization test (cherry picked from commit 675f8b7e7949211f3de9cc6d356781160f0e23e9) --- test/old_tests/UnitTests/as_implements.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/old_tests/UnitTests/as_implements.cpp b/test/old_tests/UnitTests/as_implements.cpp index 4c99631fb..ce3ef4d51 100644 --- a/test/old_tests/UnitTests/as_implements.cpp +++ b/test/old_tests/UnitTests/as_implements.cpp @@ -74,6 +74,16 @@ TEST_CASE("as_implements") REQUIRE(foo.get() == foo2.get()); } +TEST_CASE("as_implements_uniform_initialization") +{ + // uniform initialization relies on IStringable(IStringable&&), + // which requires non-const rvalue semantics. + com_ptr foo = make_self(); + IStringable stringable{ foo.as() }; + com_ptr foo2 = stringable.as(); + REQUIRE(foo.get() == foo2.get()); +} + TEST_CASE("as_implements_inheritance") { com_ptr bar = make_self(); From 42c8bb78f49b0c7e3d6309f84b134636256a24db Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Thu, 23 Feb 2023 13:05:12 -0600 Subject: [PATCH 4/4] Did you not have enough coffee? --- test/old_tests/UnitTests/as_implements.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/old_tests/UnitTests/as_implements.cpp b/test/old_tests/UnitTests/as_implements.cpp index ce3ef4d51..60b27342f 100644 --- a/test/old_tests/UnitTests/as_implements.cpp +++ b/test/old_tests/UnitTests/as_implements.cpp @@ -74,16 +74,6 @@ TEST_CASE("as_implements") REQUIRE(foo.get() == foo2.get()); } -TEST_CASE("as_implements_uniform_initialization") -{ - // uniform initialization relies on IStringable(IStringable&&), - // which requires non-const rvalue semantics. - com_ptr foo = make_self(); - IStringable stringable{ foo.as() }; - com_ptr foo2 = stringable.as(); - REQUIRE(foo.get() == foo2.get()); -} - TEST_CASE("as_implements_inheritance") { com_ptr bar = make_self(); @@ -148,3 +138,13 @@ TEST_CASE("as_implements_inheritance") REQUIRE(bar.get() == foo2.get()); } } + +TEST_CASE("convert_to_implements_via_uniform_initialization") +{ + // uniform initialization relies on IStringable(IStringable&&), + // which requires non-const rvalue semantics. + com_ptr foo = make_self(); + IStringable stringable{ *foo }; + com_ptr foo2 = stringable.as(); + REQUIRE(foo.get() == foo2.get()); +}