From dceefe27873315f9b219dce966dc2be304940f73 Mon Sep 17 00:00:00 2001 From: Raymond Chen Date: Sun, 20 Feb 2022 17:40:20 -0800 Subject: [PATCH 1/2] Improve diagnostics when `get_weak() is not supported Right now, if you call `get_weak()` from a class that does not support weak references, you get the message > This is only for weak ref support. This message appears to be some sort of internal error and creates developer confusion. Updated the error so that you get > Weak references are not supported by this class. and one (or both) of * Weak references are not supported because object does not implement IInspectable. * Weak references are not supported because no_weak_ref was specified. --- strings/base_implements.h | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/strings/base_implements.h b/strings/base_implements.h index df51f052f..fcb649acc 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -1123,9 +1123,16 @@ namespace winrt::impl return query_interface_tearoff(id, object); } + static constexpr bool assert_weak_ref_support() noexcept + { + static_assert(is_inspectable::value, "Weak references are not supported because object does not implement IInspectable."); + static_assert(!std::disjunction...>::value, "Weak references are not supported because no_weak_ref was specified."); + return is_weak_ref_source::value; + } + impl::IWeakReferenceSource* make_weak_ref() noexcept { - static_assert(is_weak_ref_source::value, "This is only for weak ref support."); + static_assert(assert_weak_ref_support(), "Weak references are not supported by this class."); uintptr_t count_or_pointer = m_references.load(std::memory_order_relaxed); if (is_weak_ref(count_or_pointer)) @@ -1163,19 +1170,19 @@ namespace winrt::impl static bool is_weak_ref(intptr_t const value) noexcept { - static_assert(is_weak_ref_source::value, "This is only for weak ref support."); + static_assert(assert_weak_ref_support(), "Weak references are not supported by this class."); return value < 0; } static weak_ref_t* decode_weak_ref(uintptr_t const value) noexcept { - static_assert(is_weak_ref_source::value, "This is only for weak ref support."); + static_assert(assert_weak_ref_support(), "Weak references are not supported by this class."); return reinterpret_cast(value << 1); } static uintptr_t encode_weak_ref(weak_ref_t* value) noexcept { - static_assert(is_weak_ref_source::value, "This is only for weak ref support."); + static_assert(assert_weak_ref_support(), "Weak references are not supported by this class."); constexpr uintptr_t pointer_flag = static_cast(1) << ((sizeof(uintptr_t) * 8) - 1); WINRT_ASSERT((reinterpret_cast(value) & 1) == 0); return (reinterpret_cast(value) >> 1) | pointer_flag; From da20494f81e53b6fea89b72371b791621436f76b Mon Sep 17 00:00:00 2001 From: Raymond Chen Date: Tue, 22 Feb 2022 14:44:03 -0800 Subject: [PATCH 2/2] Enable weak reference support for pure classic COM objects Turns out that IWeakReference does work for classic COM: Even though the output pointer of `IWeakReference::Resolve` is typed as `IInspectable**`, it is annotated as `iid_is(riid)`, so COM ignores the formal type and relies on the IID, which is not constrained to derive from IInspectable. Verified that the COM marshaling metadata for the Resolve method is unaffected by the formal type of the output pointer. In particular, querying the resulting object for `IInspectable` will send a request over the wire (if never requested before), proving the COM did not infer `IInspectable` support. This is a behavior change: Previously, pure classic COM objects did not support weak references. However, it removes developer surprise, since they see a method called `get_weak()` and expect it to work. --- strings/base_implements.h | 70 +++++++++++++++---------------- test/old_tests/UnitTests/weak.cpp | 40 ++++++++++++++++-- 2 files changed, 71 insertions(+), 39 deletions(-) diff --git a/strings/base_implements.h b/strings/base_implements.h index fcb649acc..e2c6f3a12 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -1063,7 +1063,7 @@ namespace winrt::impl using is_agile = std::negation...>>; using is_inspectable = std::disjunction...>; - using is_weak_ref_source = std::conjunction...>>>; + using is_weak_ref_source = std::negation...>>; using use_module_lock = std::negation...>>; using weak_ref_t = impl::weak_ref; @@ -1123,66 +1123,66 @@ namespace winrt::impl return query_interface_tearoff(id, object); } - static constexpr bool assert_weak_ref_support() noexcept - { - static_assert(is_inspectable::value, "Weak references are not supported because object does not implement IInspectable."); - static_assert(!std::disjunction...>::value, "Weak references are not supported because no_weak_ref was specified."); - return is_weak_ref_source::value; - } - impl::IWeakReferenceSource* make_weak_ref() noexcept { - static_assert(assert_weak_ref_support(), "Weak references are not supported by this class."); - uintptr_t count_or_pointer = m_references.load(std::memory_order_relaxed); - - if (is_weak_ref(count_or_pointer)) + if constexpr (is_weak_ref_source::value) { - return decode_weak_ref(count_or_pointer)->get_source(); - } - - com_ptr weak_ref; - *weak_ref.put() = new (std::nothrow) weak_ref_t(get_unknown(), static_cast(count_or_pointer)); + uintptr_t count_or_pointer = m_references.load(std::memory_order_relaxed); - if (!weak_ref) - { - return nullptr; - } + if (is_weak_ref(count_or_pointer)) + { + return decode_weak_ref(count_or_pointer)->get_source(); + } - uintptr_t const encoding = encode_weak_ref(weak_ref.get()); + com_ptr weak_ref; + *weak_ref.put() = new (std::nothrow) weak_ref_t(get_unknown(), static_cast(count_or_pointer)); - for (;;) - { - if (m_references.compare_exchange_weak(count_or_pointer, encoding, std::memory_order_acq_rel, std::memory_order_relaxed)) + if (!weak_ref) { - impl::IWeakReferenceSource* result = weak_ref->get_source(); - detach_abi(weak_ref); - return result; + return nullptr; } - if (is_weak_ref(count_or_pointer)) + uintptr_t const encoding = encode_weak_ref(weak_ref.get()); + + for (;;) { - return decode_weak_ref(count_or_pointer)->get_source(); - } + if (m_references.compare_exchange_weak(count_or_pointer, encoding, std::memory_order_acq_rel, std::memory_order_relaxed)) + { + impl::IWeakReferenceSource* result = weak_ref->get_source(); + detach_abi(weak_ref); + return result; + } + + if (is_weak_ref(count_or_pointer)) + { + return decode_weak_ref(count_or_pointer)->get_source(); + } - weak_ref->set_strong(static_cast(count_or_pointer)); + weak_ref->set_strong(static_cast(count_or_pointer)); + } + } + else + { + static_assert(is_weak_ref_source::value, "Weak references are not supported because no_weak_ref was specified."); + return nullptr; } } static bool is_weak_ref(intptr_t const value) noexcept { - static_assert(assert_weak_ref_support(), "Weak references are not supported by this class."); + static_assert(is_weak_ref_source::value, "Weak references are not supported because no_weak_ref was specified."); return value < 0; } static weak_ref_t* decode_weak_ref(uintptr_t const value) noexcept { - static_assert(assert_weak_ref_support(), "Weak references are not supported by this class."); + static_assert(is_weak_ref_source::value, "Weak references are not supported because no_weak_ref was specified."); return reinterpret_cast(value << 1); } static uintptr_t encode_weak_ref(weak_ref_t* value) noexcept { - static_assert(assert_weak_ref_support(), "Weak references are not supported by this class."); + static_assert(is_weak_ref_source::value, "Weak references are not supported because no_weak_ref was specified."); constexpr uintptr_t pointer_flag = static_cast(1) << ((sizeof(uintptr_t) * 8) - 1); WINRT_ASSERT((reinterpret_cast(value) & 1) == 0); return (reinterpret_cast(value) >> 1) | pointer_flag; diff --git a/test/old_tests/UnitTests/weak.cpp b/test/old_tests/UnitTests/weak.cpp index ba0fb5d86..d93377df5 100644 --- a/test/old_tests/UnitTests/weak.cpp +++ b/test/old_tests/UnitTests/weak.cpp @@ -22,7 +22,7 @@ namespace } }; - struct NoWeak : implements + struct WeakClassicCom : implements { }; @@ -161,6 +161,26 @@ TEST_CASE("weak,source") REQUIRE(b.ToString() == L"Weak"); } + SECTION("classic-com") + { + com_ptr<::IUnknown> a = make(); + + weak_ref<::IUnknown> w = a; + com_ptr<::IUnknown> b = w.get(); + REQUIRE(b == a); + + // still one outstanding reference + b = nullptr; + b = w.get(); + REQUIRE(b != nullptr); + + // no outstanding references + a = nullptr; + b = nullptr; + b = w.get(); + REQUIRE(b == nullptr); + } + // Verify that deduction guides work. static_assert(std::is_same_v, decltype(weak_ref(IStringable()))>); static_assert(std::is_same_v, decltype(weak_ref(std::declval()))>); @@ -206,12 +226,24 @@ TEST_CASE("weak,QI") REQUIRE(ref.as<::IUnknown>() != object.as<::IUnknown>()); } - SECTION("no-weak") + SECTION("weak-classic-com") { - com_ptr<::IUnknown> object = make(); + com_ptr<::IUnknown> object = make(); REQUIRE(!object.try_as()); - REQUIRE(!object.try_as()); + REQUIRE(object.try_as()); REQUIRE(!object.try_as()); + + com_ptr source = object.as(); + REQUIRE(!source.try_as()); + REQUIRE(source.try_as()); + REQUIRE(object.as<::IUnknown>() == source.as<::IUnknown>()); + + com_ptr ref; + REQUIRE(S_OK == source->GetWeakReference(ref.put())); + REQUIRE(!ref.try_as()); + REQUIRE(!ref.try_as()); + REQUIRE(ref.as() == ref); + REQUIRE(ref.as<::IUnknown>() != object.as<::IUnknown>()); } SECTION("factory")