From 472e8d4b2590cdbcda361695be4efb8f4c41e3aa Mon Sep 17 00:00:00 2001 From: Antonio M <43587397+antmor@users.noreply.github.com> Date: Wed, 13 Dec 2023 16:16:24 -0800 Subject: [PATCH 01/18] non-originating error compiles --- cppwinrt/code_writers.h | 14 ++++++++ strings/base_collections_base.h | 37 ++++++++++++++++++-- strings/base_collections_input_map.h | 21 ++++++------ strings/base_collections_map.h | 51 ++++++++++++++-------------- strings/base_coroutine_foundation.h | 25 ++++++++++++-- strings/base_coroutine_threadpool.h | 11 ++++++ strings/base_error.h | 27 +++++++++++++-- 7 files changed, 144 insertions(+), 42 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 3c77269cd..1ffd45efd 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1368,6 +1368,11 @@ namespace cppwinrt w.write(R"( auto TryLookup(param_type const& key) const { + //if (auto ignoreError = as()) + //{ + // ignoreError->IgnoreNextError(); + //} + if constexpr (std::is_base_of_v) { V result{ nullptr }; @@ -1394,6 +1399,11 @@ namespace cppwinrt w.write(R"( auto TryLookup(param_type const& key) const { + //if (auto ignoreError = as()) + //{ + // ignoreError->IgnoreNextError(); + //} + if constexpr (std::is_base_of_v) { V result{ nullptr }; @@ -1416,6 +1426,10 @@ namespace cppwinrt auto TryRemove(param_type const& key) const { + /*if (auto ignoreError = as()) + { + ignoreError->IgnoreNextError(); + }*/ return 0 == impl::check_hresult_allow_bounds(WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMap)->Remove(get_abi(key))); } )"); diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index b2d58f602..bc61141bc 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -77,6 +77,15 @@ namespace winrt::impl }; } +//struct WINRT_IMPL_NOVTABLE IIgnoreNextError : unknown_abi +//{ +// virtual void __stdcall IgnoreNextError() noexcept = 0; +//}; +// +//template <> inline constexpr guid guid_v{ 0x5b0d3235, 0x4dba, 0x4d44, { 0x86,0x5e,0x8f,0x1d,0x0e,0x4f,0xd0,0x4d } }; +// + + WINRT_EXPORT namespace winrt { template @@ -513,7 +522,14 @@ WINRT_EXPORT namespace winrt if (pair == static_cast(*this).get_container().end()) { - throw hresult_out_of_bounds(); + if (m_dontOriginateBoundsError) + { + throw non_originating_hresult_out_of_bounds(); + } + else + { + throw hresult_out_of_bounds(); + } } return static_cast(*this).unwrap_value(pair->second); @@ -536,6 +552,16 @@ WINRT_EXPORT namespace winrt first = nullptr; second = nullptr; } + + // // IIgnoreNextError + // void IgnoreNextError() + // { + // m_ignoreNextError = true; + // } + //private: + // bool m_ignoreNextError{ false }; + protected: + bool m_dontOriginateBoundsError{ false }; }; template @@ -571,7 +597,14 @@ WINRT_EXPORT namespace winrt auto found = container.find(static_cast(*this).wrap_value(key)); if (found == container.end()) { - throw hresult_out_of_bounds(); + if (map_view_base::m_dontOriginateBoundsError) + { + throw non_originating_hresult_out_of_bounds(); + } + else + { + throw hresult_out_of_bounds(); + } } this->increment_version(); removedNode = container.extract(found); diff --git a/strings/base_collections_input_map.h b/strings/base_collections_input_map.h index b33975fe6..525b383c1 100644 --- a/strings/base_collections_input_map.h +++ b/strings/base_collections_input_map.h @@ -9,8 +9,9 @@ namespace winrt::impl { static_assert(std::is_same_v>, "Must be constructed with rvalue."); - explicit map_impl(Container&& values) : m_values(std::forward(values)) - { + explicit map_impl(Container&& values, bool dontOriginateError = false) : m_values(std::forward(values)) + { + this->m_dontOriginateBoundsError = dontOriginateError; } auto& get_container() noexcept @@ -35,9 +36,9 @@ namespace winrt::impl using input_map = map_impl; template - auto make_input_map(Container&& values) + auto make_input_map(Container&& values, bool dontOriginateError = false) { - return make>(std::forward(values)); + return make>(std::forward(values), dontOriginateError); } } @@ -68,19 +69,19 @@ WINRT_EXPORT namespace winrt::param } template - map(std::map&& values) : - m_interface(impl::make_input_map(std::move(values))) + map(std::map&& values, bool dontOriginateError = false) : + m_interface(impl::make_input_map(std::move(values), dontOriginateError)) { } template - map(std::unordered_map&& values) : - m_interface(impl::make_input_map(std::move(values))) + map(std::unordered_map&& values, bool dontOriginateError = false) : + m_interface(impl::make_input_map(std::move(values), dontOriginateError)) { } - map(std::initializer_list> values) : - m_interface(impl::make_input_map(std::map(values))) + map(std::initializer_list> values, bool dontOriginateError = false) : + m_interface(impl::make_input_map(std::map(values, dontOriginateError))) { } diff --git a/strings/base_collections_map.h b/strings/base_collections_map.h index f4bd74baf..a00f17ade 100644 --- a/strings/base_collections_map.h +++ b/strings/base_collections_map.h @@ -12,8 +12,9 @@ namespace winrt::impl { static_assert(std::is_same_v>, "Must be constructed with rvalue."); - explicit observable_map_impl(Container&& values) : m_values(std::forward(values)) + explicit observable_map_impl(Container&& values, bool dontOriginateError = false) : m_values(std::forward(values)) { + this->m_dontOriginateBoundsError = dontOriginateError; } auto& get_container() noexcept @@ -44,75 +45,75 @@ namespace winrt::impl WINRT_EXPORT namespace winrt { template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map() + Windows::Foundation::Collections::IMap single_threaded_map(bool dontOriginateError = false) { - return make>>(std::map{}); + return make>>(std::map{}, dontOriginateError); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map(std::map&& values) + Windows::Foundation::Collections::IMap single_threaded_map(std::map&& values, bool dontOriginateError = false) { - return make>>(std::move(values)); + return make>>(std::move(values), dontOriginateError); } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map(std::unordered_map&& values) + Windows::Foundation::Collections::IMap single_threaded_map(std::unordered_map&& values, bool dontOriginateError = false) { - return make>>(std::move(values)); + return make>>(std::move(values), dontOriginateError); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map() + Windows::Foundation::Collections::IMap multi_threaded_map(bool dontOriginateError = false) { - return make>>(std::map{}); + return make>>(std::map{}, dontOriginateError); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map(std::map&& values) + Windows::Foundation::Collections::IMap multi_threaded_map(std::map&& values, bool dontOriginateError = false) { - return make>>(std::move(values)); + return make>>(std::move(values), dontOriginateError); } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map(std::unordered_map&& values) + Windows::Foundation::Collections::IMap multi_threaded_map(std::unordered_map&& values, bool dontOriginateError = false) { - return make>>(std::move(values)); + return make>>(std::move(values), dontOriginateError); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map() + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(bool dontOriginateError = false) { - return make>>(std::map{}); + return make>>(std::map{}, dontOriginateError); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::map&& values) + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::map&& values, bool dontOriginateError = false) { - return make>>(std::move(values)); + return make>>(std::move(values), dontOriginateError); } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::unordered_map&& values) + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::unordered_map&& values, bool dontOriginateError = false) { - return make>>(std::move(values)); + return make>>(std::move(values), dontOriginateError); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map() + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(bool dontOriginateError = false) { - return make>>(std::map{}); + return make>>(std::map{}, dontOriginateError); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::map&& values) + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::map&& values, bool dontOriginateError = false) { - return make>>(std::move(values)); + return make>>(std::move(values), dontOriginateError); } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::unordered_map&& values) + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::unordered_map&& values, bool dontOriginateError = false) { - return make>>(std::move(values)); + return make>>(std::move(values), dontOriginateError); } } diff --git a/strings/base_coroutine_foundation.h b/strings/base_coroutine_foundation.h index 670a65403..a9005f9da 100644 --- a/strings/base_coroutine_foundation.h +++ b/strings/base_coroutine_foundation.h @@ -202,10 +202,11 @@ namespace winrt::impl static fire_and_forget cancel_asynchronously(Async async) { + auto asyncRef = async; co_await winrt::resume_background(); try { - async.Cancel(); + asyncRef.Cancel(); } catch (hresult_error const&) { @@ -351,6 +352,10 @@ namespace winrt::impl return m_promise->enable_cancellation_propagation(value); } + bool avoid_logging(bool value = true) const noexcept + { + return m_promise->enable_avoid_rooriginate_cancel(value); + } private: Promise* m_promise; @@ -484,7 +489,14 @@ namespace winrt::impl if (m_status.load(std::memory_order_relaxed) == AsyncStatus::Started) { m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed); - m_exception = std::make_exception_ptr(hresult_canceled()); + if (cancellable_promise::avoid_rooriginate_cancel_enabled()) + { + m_exception = std::make_exception_ptr(non_originating_hresult_canceled()); + } + else + { + m_exception = std::make_exception_ptr(hresult_canceled()); + } cancel = std::move(m_cancel); } } @@ -628,7 +640,14 @@ namespace winrt::impl { if (Status() == AsyncStatus::Canceled) { - throw winrt::hresult_canceled(); + if (cancellable_promise::avoid_rooriginate_cancel_enabled()) + { + throw winrt::non_originating_hresult_canceled(); + } + else + { + throw winrt::hresult_canceled(); + } } return std::forward(expression); diff --git a/strings/base_coroutine_threadpool.h b/strings/base_coroutine_threadpool.h index 0faaa1acd..117af8d59 100644 --- a/strings/base_coroutine_threadpool.h +++ b/strings/base_coroutine_threadpool.h @@ -180,12 +180,23 @@ WINRT_EXPORT namespace winrt return m_propagate_cancellation; } + bool enable_avoid_rooriginate_cancel(bool value) noexcept + { + return std::exchange(m_avoid_rooriginate_cancel, value); + } + + bool avoid_rooriginate_cancel_enabled() const noexcept + { + return m_avoid_rooriginate_cancel; + } + private: static inline auto const cancelling_ptr = reinterpret_cast(1); std::atomic m_canceller{ nullptr }; void* m_context{ nullptr }; bool m_propagate_cancellation{ false }; + bool m_avoid_rooriginate_cancel{ false }; }; template diff --git a/strings/base_error.h b/strings/base_error.h index 2a6eea281..9996cf0a8 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -294,7 +294,7 @@ WINRT_EXPORT namespace winrt return m_code; } - private: + protected: void originate(hresult const code, void* message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept { @@ -315,7 +315,7 @@ WINRT_EXPORT namespace winrt com_ptr info; WINRT_VERIFY_(0, WINRT_IMPL_GetErrorInfo(0, info.put_void())); WINRT_VERIFY(info.try_as(m_info)); - } + } static hresult verify_error(hresult const code) noexcept { @@ -339,6 +339,29 @@ WINRT_EXPORT namespace winrt #endif }; + struct non_originating_hresult_error : hresult_error + { + protected: + void originate(hresult const /*code*/, void* /*message*/ WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept + { + // override and do nothing. + } + }; + + struct non_originating_hresult_canceled : hresult_error + { + non_originating_hresult_canceled(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_access_denied WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_canceled(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_access_denied, message WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_canceled(take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_access_denied, take_ownership_from_abi WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + }; + + struct non_originating_hresult_out_of_bounds : hresult_error + { + non_originating_hresult_out_of_bounds(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_access_denied WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_out_of_bounds(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_access_denied, message WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_out_of_bounds(take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_access_denied, take_ownership_from_abi WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + }; + struct hresult_access_denied : hresult_error { hresult_access_denied(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_access_denied WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} From e358b9296d28b082ef14c14d13afdb0f23e502fd Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Thu, 14 Dec 2023 18:00:29 -0800 Subject: [PATCH 02/18] fix with test cases --- strings/base_collections_input_map.h | 2 +- strings/base_coroutine_foundation.h | 4 ++ strings/base_error.h | 53 +++++++++-------- test/test/async_check_cancel.cpp | 86 ++++++++++++++++++++++++++-- test/test_cpp20/custom_error.cpp | 27 +++++++++ 5 files changed, 143 insertions(+), 29 deletions(-) diff --git a/strings/base_collections_input_map.h b/strings/base_collections_input_map.h index 525b383c1..75c7c8688 100644 --- a/strings/base_collections_input_map.h +++ b/strings/base_collections_input_map.h @@ -81,7 +81,7 @@ WINRT_EXPORT namespace winrt::param } map(std::initializer_list> values, bool dontOriginateError = false) : - m_interface(impl::make_input_map(std::map(values, dontOriginateError))) + m_interface(impl::make_input_map(std::map(values), dontOriginateError)) { } diff --git a/strings/base_coroutine_foundation.h b/strings/base_coroutine_foundation.h index a9005f9da..6d9c41e4f 100644 --- a/strings/base_coroutine_foundation.h +++ b/strings/base_coroutine_foundation.h @@ -629,6 +629,10 @@ namespace winrt::impl { m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed); } + catch (non_originating_hresult_canceled const&) + { + m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed); + } catch (...) { m_status.store(AsyncStatus::Error, std::memory_order_relaxed); diff --git a/strings/base_error.h b/strings/base_error.h index 9996cf0a8..45d350114 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -206,11 +206,27 @@ WINRT_EXPORT namespace winrt originate(code, nullptr WINRT_IMPL_SOURCE_LOCATION_FORWARD); } + explicit hresult_error(hresult const code, bool avoidOrigination WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) + { + if (!avoidOrigination) + { + originate(code, nullptr WINRT_IMPL_SOURCE_LOCATION_FORWARD); + } + } + hresult_error(hresult const code, param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) { originate(code, get_abi(message) WINRT_IMPL_SOURCE_LOCATION_FORWARD); } + hresult_error(hresult const code, param::hstring const& message, bool avoidOrigination WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) + { + if (!avoidOrigination) + { + originate(code, get_abi(message) WINRT_IMPL_SOURCE_LOCATION_FORWARD); + } + } + hresult_error(hresult const code, take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) { com_ptr info; @@ -294,7 +310,7 @@ WINRT_EXPORT namespace winrt return m_code; } - protected: + private: void originate(hresult const code, void* message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept { @@ -339,29 +355,6 @@ WINRT_EXPORT namespace winrt #endif }; - struct non_originating_hresult_error : hresult_error - { - protected: - void originate(hresult const /*code*/, void* /*message*/ WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept - { - // override and do nothing. - } - }; - - struct non_originating_hresult_canceled : hresult_error - { - non_originating_hresult_canceled(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_access_denied WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} - non_originating_hresult_canceled(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_access_denied, message WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} - non_originating_hresult_canceled(take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_access_denied, take_ownership_from_abi WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} - }; - - struct non_originating_hresult_out_of_bounds : hresult_error - { - non_originating_hresult_out_of_bounds(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_access_denied WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} - non_originating_hresult_out_of_bounds(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_access_denied, message WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} - non_originating_hresult_out_of_bounds(take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_access_denied, take_ownership_from_abi WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} - }; - struct hresult_access_denied : hresult_error { hresult_access_denied(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_access_denied WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} @@ -453,6 +446,18 @@ WINRT_EXPORT namespace winrt hresult_canceled(take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_canceled, take_ownership_from_abi WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} }; + struct non_originating_hresult_canceled : hresult_error + { + non_originating_hresult_canceled(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_canceled, true /*don't originate*/ WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_canceled(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_canceled, message, true /*don't originate*/ WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + }; + + struct non_originating_hresult_out_of_bounds : hresult_error + { + non_originating_hresult_out_of_bounds(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_out_of_bounds, true /*don't originate*/ WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_out_of_bounds(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_out_of_bounds, message, true /*don't originate*/ WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + }; + [[noreturn]] inline WINRT_IMPL_NOINLINE void throw_hresult(hresult const result WINRT_IMPL_SOURCE_LOCATION_ARGS) { if (winrt_throw_hresult_handler) diff --git a/test/test/async_check_cancel.cpp b/test/test/async_check_cancel.cpp index 7547609f6..b9373532c 100644 --- a/test/test/async_check_cancel.cpp +++ b/test/test/async_check_cancel.cpp @@ -11,6 +11,28 @@ namespace using std::experimental::suspend_never; #endif + static bool s_exceptionLoggerCalled = false; + + static struct { + uint32_t lineNumber; + char const* fileName; + char const* functionName; + void* returnAddress; + winrt::hresult result; + } s_exceptionLoggerArgs{}; + + void __stdcall exceptionLogger(uint32_t lineNumber, char const* fileName, char const* functionName, void* returnAddress, winrt::hresult const result) noexcept + { + s_exceptionLoggerArgs = { + /*.lineNumber =*/ lineNumber, + /*.fileName =*/ fileName, + /*.functionName =*/ functionName, + /*.returnAddress =*/ returnAddress, + /*.result =*/ result, + }; + s_exceptionLoggerCalled = true; + } + // // Checks that manual cancellation checks work. // @@ -61,6 +83,7 @@ namespace co_return 1; } + IAsyncOperationWithProgress OperationWithProgress(HANDLE event, bool& canceled) { co_await resume_on_signal(event); @@ -77,6 +100,59 @@ namespace co_return 1; } + IAsyncAction OperationCancelLogged(HANDLE event, bool& canceled) + { + REQUIRE(!s_exceptionLoggerCalled); + REQUIRE(!winrt_throw_hresult_handler); + winrt_throw_hresult_handler = exceptionLogger; + + co_await resume_on_signal(event); + auto cancel = co_await get_cancellation_token(); + cancel.avoid_logging(true); + + if (cancel()) + { + REQUIRE(!canceled); + canceled = true; + REQUIRE(s_exceptionLoggerCalled); + REQUIRE(s_exceptionLoggerArgs.result == HRESULT_FROM_WIN32(ERROR_CANCELLED)); + } + + winrt_throw_hresult_handler = nullptr; + s_exceptionLoggerCalled = false; + + co_await suspend_never(); + + REQUIRE(false); + co_return; + } + + IAsyncAction OperationAvoidLoggingCancel(HANDLE event, bool& canceled) + { + REQUIRE(!s_exceptionLoggerCalled); + REQUIRE(!winrt_throw_hresult_handler); + winrt_throw_hresult_handler = exceptionLogger; + + co_await resume_on_signal(event); + auto cancel = co_await get_cancellation_token(); + cancel.avoid_logging(true); + + if (cancel()) + { + REQUIRE(!canceled); + canceled = true; + REQUIRE(!s_exceptionLoggerCalled); + } + + winrt_throw_hresult_handler = nullptr; + s_exceptionLoggerCalled = false; + + co_await suspend_never(); + + REQUIRE(false); + co_return; + } + template void Check(F make) { @@ -111,8 +187,10 @@ TEST_CASE("async_check_cancel", "[.clang-crash]") TEST_CASE("async_check_cancel") #endif { - Check(Action); - Check(ActionWithProgress); - Check(Operation); - Check(OperationWithProgress); + //Check(Action); + //Check(ActionWithProgress); + //Check(Operation); + //Check(OperationWithProgress); + Check(OperationCancelLogged); + Check(OperationAvoidLoggingCancel); } diff --git a/test/test_cpp20/custom_error.cpp b/test/test_cpp20/custom_error.cpp index 97bfd8dc9..b674c6905 100644 --- a/test/test_cpp20/custom_error.cpp +++ b/test/test_cpp20/custom_error.cpp @@ -2,6 +2,7 @@ using namespace winrt; using namespace Windows::Foundation; +using namespace Windows::Foundation::Collections; namespace { @@ -64,6 +65,32 @@ TEST_CASE("custom_error_logger") REQUIRE(s_loggerArgs.returnAddress); REQUIRE(s_loggerArgs.result == static_cast(0x80000018)); // E_ILLEGAL_DELEGATE_ASSIGNMENT) + s_loggerCalled = false; + + { + // Log a try lookup + IMap c = single_threaded_map(); + c.Insert(L"hello"sv, 1); + c.Lookup(L"hello"sv); + auto val = c.TryLookup(L"hello"sv); + REQUIRE(val); + c.Remove(L"hello"sv); + REQUIRE(!s_loggerCalled); + // Now after we remove the item, TryLookup will fail and the logger will be called. + + val = c.TryLookup(L"hello"sv); + REQUIRE(!val); + REQUIRE(s_loggerCalled); + + // now create an empty map that _doesn't_ log + s_loggerCalled = false; + IMap c2 = single_threaded_map(std::map{}, true); + val = c2.TryLookup(L"hello"sv); + REQUIRE(!val); + // ensure that the logger wasn't called + REQUIRE(!s_loggerCalled); + } + // Remove global handler winrt_throw_hresult_handler = nullptr; s_loggerCalled = false; From a6ab125ae921dfd1fc9e98764cdf5b8ffe824625 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Thu, 14 Dec 2023 18:02:33 -0800 Subject: [PATCH 03/18] and remove ignore --- cppwinrt/code_writers.h | 14 -------------- strings/base_collections_base.h | 9 --------- 2 files changed, 23 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 1ffd45efd..3c77269cd 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1368,11 +1368,6 @@ namespace cppwinrt w.write(R"( auto TryLookup(param_type const& key) const { - //if (auto ignoreError = as()) - //{ - // ignoreError->IgnoreNextError(); - //} - if constexpr (std::is_base_of_v) { V result{ nullptr }; @@ -1399,11 +1394,6 @@ namespace cppwinrt w.write(R"( auto TryLookup(param_type const& key) const { - //if (auto ignoreError = as()) - //{ - // ignoreError->IgnoreNextError(); - //} - if constexpr (std::is_base_of_v) { V result{ nullptr }; @@ -1426,10 +1416,6 @@ namespace cppwinrt auto TryRemove(param_type const& key) const { - /*if (auto ignoreError = as()) - { - ignoreError->IgnoreNextError(); - }*/ return 0 == impl::check_hresult_allow_bounds(WINRT_IMPL_SHIM(Windows::Foundation::Collections::IMap)->Remove(get_abi(key))); } )"); diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index bc61141bc..e233fe557 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -77,15 +77,6 @@ namespace winrt::impl }; } -//struct WINRT_IMPL_NOVTABLE IIgnoreNextError : unknown_abi -//{ -// virtual void __stdcall IgnoreNextError() noexcept = 0; -//}; -// -//template <> inline constexpr guid guid_v{ 0x5b0d3235, 0x4dba, 0x4d44, { 0x86,0x5e,0x8f,0x1d,0x0e,0x4f,0xd0,0x4d } }; -// - - WINRT_EXPORT namespace winrt { template From cefa5fa191b00c8eae228b1c512d4a590d740a57 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Mon, 18 Dec 2023 14:54:19 -0800 Subject: [PATCH 04/18] add runsettings to run tests from visual studio tester, modified readme, make shouldOrigiante a template param. --- .runsettings | 32 ++++++++++++ README.md | 3 ++ strings/base_collections_base.h | 37 +++++-------- strings/base_collections_input_map.h | 39 +++++++------- strings/base_collections_input_map_view.h | 20 +++---- strings/base_collections_map.h | 63 +++++++++++------------ 6 files changed, 107 insertions(+), 87 deletions(-) create mode 100644 .runsettings diff --git a/.runsettings b/.runsettings new file mode 100644 index 000000000..b5a55c79f --- /dev/null +++ b/.runsettings @@ -0,0 +1,32 @@ + + + + 1 + .\TestResults + 60000 + true + + + + + + + + Verbose + ^test + true + 30000 + on + true + Normal + StatsOnly + ShortInfo + , + 20000 + + + 10 + + + + \ No newline at end of file diff --git a/README.md b/README.md index 691809d31..6426f3522 100644 --- a/README.md +++ b/README.md @@ -34,3 +34,6 @@ a dev command prompt at the root of the repo _after_ following the above build i * Run `build_prior_projection.cmd` in the dev command prompt as well * Run `prepare_versionless_diffs.cmd` which removes version stamps on both current and prior projection * Use a directory-level differencing tool to compare `_build\$(arch)\$(flavor)\winrt` and `_reference\$(arch)\$(flavor)\winrt` + +## Testing +This repository uses the [Catch2](https://github.com/catchorg/Catch2) testing framework. You can run `build_tests_all.cmd` to run the test from a Visual Studio command line. This will build and run the tests. To Debug the tests, you can debug the associated `_build\$(arch)\$(flavor)\.exe` under the debugger of your choice. \ No newline at end of file diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index e233fe557..06c52ef7c 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -503,7 +503,7 @@ WINRT_EXPORT namespace winrt }; }; - template + template struct map_view_base : iterable_base, Version> { V Lookup(K const& key) const @@ -513,13 +513,13 @@ WINRT_EXPORT namespace winrt if (pair == static_cast(*this).get_container().end()) { - if (m_dontOriginateBoundsError) + if constexpr (ShouldOriginate) { - throw non_originating_hresult_out_of_bounds(); + throw hresult_out_of_bounds(); } else { - throw hresult_out_of_bounds(); + throw non_originating_hresult_out_of_bounds(); } } @@ -544,19 +544,10 @@ WINRT_EXPORT namespace winrt second = nullptr; } - // // IIgnoreNextError - // void IgnoreNextError() - // { - // m_ignoreNextError = true; - // } - //private: - // bool m_ignoreNextError{ false }; - protected: - bool m_dontOriginateBoundsError{ false }; }; - template - struct map_base : map_view_base + template + struct map_base : map_view_base { Windows::Foundation::Collections::IMapView GetView() const { @@ -588,13 +579,13 @@ WINRT_EXPORT namespace winrt auto found = container.find(static_cast(*this).wrap_value(key)); if (found == container.end()) { - if (map_view_base::m_dontOriginateBoundsError) + if constexpr (ShouldOriginate) { - throw non_originating_hresult_out_of_bounds(); + throw hresult_out_of_bounds(); } else { - throw hresult_out_of_bounds(); + throw non_originating_hresult_out_of_bounds(); } } this->increment_version(); @@ -611,8 +602,8 @@ WINRT_EXPORT namespace winrt } }; - template - struct observable_map_base : map_base + template + struct observable_map_base : map_base { event_token MapChanged(Windows::Foundation::Collections::MapChangedEventHandler const& handler) { @@ -626,20 +617,20 @@ WINRT_EXPORT namespace winrt bool Insert(K const& key, V const& value) { - bool const result = map_base::Insert(key, value); + bool const result = map_base::Insert(key, value); call_changed(Windows::Foundation::Collections::CollectionChange::ItemInserted, key); return result; } void Remove(K const& key) { - map_base::Remove(key); + map_base::Remove(key); call_changed(Windows::Foundation::Collections::CollectionChange::ItemRemoved, key); } void Clear() noexcept { - map_base::Clear(); + map_base::Clear(); call_changed(Windows::Foundation::Collections::CollectionChange::Reset, impl::empty_value()); } diff --git a/strings/base_collections_input_map.h b/strings/base_collections_input_map.h index 75c7c8688..8cd54c826 100644 --- a/strings/base_collections_input_map.h +++ b/strings/base_collections_input_map.h @@ -1,18 +1,15 @@ namespace winrt::impl { - template + template struct map_impl : - implements, wfc::IMap, wfc::IMapView, wfc::IIterable>>, - map_base, K, V>, + implements, wfc::IMap, wfc::IMapView, wfc::IIterable>>, + map_base, K, V, ShouldOriginate>, ThreadingBase { static_assert(std::is_same_v>, "Must be constructed with rvalue."); - explicit map_impl(Container&& values, bool dontOriginateError = false) : m_values(std::forward(values)) - { - this->m_dontOriginateBoundsError = dontOriginateError; - } + explicit map_impl(Container&& values) : m_values(std::forward(values)) {} auto& get_container() noexcept { @@ -32,19 +29,19 @@ namespace winrt::impl Container m_values; }; - template - using input_map = map_impl; + template + using input_map = map_impl; - template - auto make_input_map(Container&& values, bool dontOriginateError = false) + template + auto make_input_map(Container&& values) { - return make>(std::forward(values), dontOriginateError); + return make>(std::forward(values)); } } WINRT_EXPORT namespace winrt::param { - template + template struct map { using value_type = Windows::Foundation::Collections::IKeyValuePair; @@ -69,19 +66,19 @@ WINRT_EXPORT namespace winrt::param } template - map(std::map&& values, bool dontOriginateError = false) : - m_interface(impl::make_input_map(std::move(values), dontOriginateError)) + map(std::map&& values) : + m_interface(impl::make_input_map, ShouldOriginate>(std::move(values))) { } template - map(std::unordered_map&& values, bool dontOriginateError = false) : - m_interface(impl::make_input_map(std::move(values), dontOriginateError)) + map(std::unordered_map&& values) : + m_interface(impl::make_input_map, ShouldOriginate>(std::move(values))) { } - map(std::initializer_list> values, bool dontOriginateError = false) : - m_interface(impl::make_input_map(std::map(values), dontOriginateError)) + map(std::initializer_list> values) : + m_interface(impl::make_input_map, ShouldOriginate>(std::map(values))) { } @@ -104,8 +101,8 @@ WINRT_EXPORT namespace winrt::param bool m_owned{ true }; }; - template - auto get_abi(map const& object) noexcept + template + auto get_abi(map const& object) noexcept { return *(void**)(&object); } diff --git a/strings/base_collections_input_map_view.h b/strings/base_collections_input_map_view.h index bfd8d82a9..b7fc9d30f 100644 --- a/strings/base_collections_input_map_view.h +++ b/strings/base_collections_input_map_view.h @@ -1,10 +1,10 @@ namespace winrt::impl { - template + template struct input_map_view : - implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, - map_view_base, K, V> + implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, + map_view_base, K, V, ShouldOriginate> { static_assert(std::is_same_v>, "Must be constructed with rvalue."); @@ -22,11 +22,11 @@ namespace winrt::impl Container const m_values; }; - template + template struct scoped_input_map_view : input_scope, - implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, - map_view_base, K, V> + implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, + map_view_base, K, V, ShouldOriginate> { void abi_enter() const { @@ -53,18 +53,18 @@ namespace winrt::impl Container const& m_values; }; - template + template auto make_input_map_view(Container&& values) { - return make>(std::forward(values)); + return make>(std::forward(values)); } - template + template auto make_scoped_input_map_view(Container const& values) { using interface_type = wfc::IMapView; std::pair result; - auto ptr = new scoped_input_map_view(values); + auto ptr = new scoped_input_map_view(values); *put_abi(result.first) = to_abi(ptr); result.second = ptr; return result; diff --git a/strings/base_collections_map.h b/strings/base_collections_map.h index a00f17ade..58d2e0108 100644 --- a/strings/base_collections_map.h +++ b/strings/base_collections_map.h @@ -1,21 +1,18 @@ namespace winrt::impl { - template - using multi_threaded_map = map_impl; + template + using multi_threaded_map = map_impl; - template + template struct observable_map_impl : - implements, wfc::IObservableMap, wfc::IMap, wfc::IMapView, wfc::IIterable>>, - observable_map_base, K, V>, + implements, wfc::IObservableMap, wfc::IMap, wfc::IMapView, wfc::IIterable>>, + observable_map_base, K, V, ShouldOriginate>, ThreadingBase { static_assert(std::is_same_v>, "Must be constructed with rvalue."); - explicit observable_map_impl(Container&& values, bool dontOriginateError = false) : m_values(std::forward(values)) - { - this->m_dontOriginateBoundsError = dontOriginateError; - } + explicit observable_map_impl(Container&& values) : m_values(std::forward(values)) {} auto& get_container() noexcept { @@ -45,75 +42,75 @@ namespace winrt::impl WINRT_EXPORT namespace winrt { template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map(bool dontOriginateError = false) + Windows::Foundation::Collections::IMap single_threaded_map() { - return make>>(std::map{}, dontOriginateError); + return make>>(std::map{}); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map(std::map&& values, bool dontOriginateError = false) + Windows::Foundation::Collections::IMap single_threaded_map(std::map&& values) { - return make>>(std::move(values), dontOriginateError); + return make>>(std::move(values)); } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map(std::unordered_map&& values, bool dontOriginateError = false) + Windows::Foundation::Collections::IMap single_threaded_map(std::unordered_map&& values) { - return make>>(std::move(values), dontOriginateError); + return make>>(std::move(values)); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map(bool dontOriginateError = false) + Windows::Foundation::Collections::IMap multi_threaded_map() { - return make>>(std::map{}, dontOriginateError); + return make>>(std::map{}); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map(std::map&& values, bool dontOriginateError = false) + Windows::Foundation::Collections::IMap multi_threaded_map(std::map&& values) { - return make>>(std::move(values), dontOriginateError); + return make>>(std::move(values)); } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map(std::unordered_map&& values, bool dontOriginateError = false) + Windows::Foundation::Collections::IMap multi_threaded_map(std::unordered_map&& values) { - return make>>(std::move(values), dontOriginateError); + return make>>(std::move(values)); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(bool dontOriginateError = false) + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map() { - return make>>(std::map{}, dontOriginateError); + return make>>(std::map{}); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::map&& values, bool dontOriginateError = false) + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::map&& values) { - return make>>(std::move(values), dontOriginateError); + return make>>(std::move(values)); } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::unordered_map&& values, bool dontOriginateError = false) + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::unordered_map&& values) { - return make>>(std::move(values), dontOriginateError); + return make>>(std::move(values)); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(bool dontOriginateError = false) + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map() { - return make>>(std::map{}, dontOriginateError); + return make>>(std::map{}); } template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::map&& values, bool dontOriginateError = false) + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::map&& values) { - return make>>(std::move(values), dontOriginateError); + return make>>(std::move(values)); } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::unordered_map&& values, bool dontOriginateError = false) + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::unordered_map&& values) { - return make>>(std::move(values), dontOriginateError); + return make>>(std::move(values)); } } From a66ecd3099d3cf25bf0112c89e5d34f55fa637f3 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Mon, 18 Dec 2023 19:07:10 -0800 Subject: [PATCH 05/18] change map shouldOriginate to template parameter. Note, could be a breakig change for usages of map that are not default (i.e. custom std::less) if so, might need to split up templates into separate template, or spin out into separate pr. changed bool to template parameter in hresult_error, as the char*-to-bool decaying conversion failed the old tests. also fixed runsettings, added note in readme. --- .runsettings | 5 -- README.md | 4 +- strings/base_collections_base.h | 2 +- strings/base_collections_input_map.h | 14 ++--- strings/base_collections_map.h | 60 +++++++++++----------- strings/base_error.h | 28 ++++------ test/old_tests/UnitTests/Errors.cpp | 1 + test/old_tests/UnitTests/hresult_error.cpp | 3 +- test/test/async_check_cancel.cpp | 14 ++--- test/test_cpp20/custom_error.cpp | 2 +- 10 files changed, 64 insertions(+), 69 deletions(-) diff --git a/.runsettings b/.runsettings index b5a55c79f..6de2a3bec 100644 --- a/.runsettings +++ b/.runsettings @@ -23,10 +23,5 @@ ShortInfo , 20000 - - - 10 - - \ No newline at end of file diff --git a/README.md b/README.md index 6426f3522..d10bcbe57 100644 --- a/README.md +++ b/README.md @@ -36,4 +36,6 @@ a dev command prompt at the root of the repo _after_ following the above build i * Use a directory-level differencing tool to compare `_build\$(arch)\$(flavor)\winrt` and `_reference\$(arch)\$(flavor)\winrt` ## Testing -This repository uses the [Catch2](https://github.com/catchorg/Catch2) testing framework. You can run `build_tests_all.cmd` to run the test from a Visual Studio command line. This will build and run the tests. To Debug the tests, you can debug the associated `_build\$(arch)\$(flavor)\.exe` under the debugger of your choice. \ No newline at end of file +This repository uses the [Catch2](https://github.com/catchorg/Catch2) testing framework. +- From a Visual Studio command line, you should run `build_tests_all.cmd` to build and run the tests. To Debug the tests, you can debug the associated `_build\$(arch)\$(flavor)\.exe` under the debugger of your choice. +- Optionally, you can install the [Catch2Adapter](https://marketplace.visualstudio.com/items?itemName=JohnnyHendriks.ext01) to run the tests from Visual Studio. \ No newline at end of file diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index 06c52ef7c..861a2602f 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -602,7 +602,7 @@ WINRT_EXPORT namespace winrt } }; - template + template struct observable_map_base : map_base { event_token MapChanged(Windows::Foundation::Collections::MapChangedEventHandler const& handler) diff --git a/strings/base_collections_input_map.h b/strings/base_collections_input_map.h index 8cd54c826..2a17d83d2 100644 --- a/strings/base_collections_input_map.h +++ b/strings/base_collections_input_map.h @@ -29,7 +29,7 @@ namespace winrt::impl Container m_values; }; - template + template using input_map = map_impl; template @@ -41,7 +41,7 @@ namespace winrt::impl WINRT_EXPORT namespace winrt::param { - template + template struct map { using value_type = Windows::Foundation::Collections::IKeyValuePair; @@ -67,18 +67,18 @@ WINRT_EXPORT namespace winrt::param template map(std::map&& values) : - m_interface(impl::make_input_map, ShouldOriginate>(std::move(values))) + m_interface(impl::make_input_map(std::move(values))) { } template map(std::unordered_map&& values) : - m_interface(impl::make_input_map, ShouldOriginate>(std::move(values))) + m_interface(impl::make_input_map(std::move(values))) { } map(std::initializer_list> values) : - m_interface(impl::make_input_map, ShouldOriginate>(std::map(values))) + m_interface(impl::make_input_map(std::map(values))) { } @@ -101,8 +101,8 @@ WINRT_EXPORT namespace winrt::param bool m_owned{ true }; }; - template - auto get_abi(map const& object) noexcept + template + auto get_abi(map const& object) noexcept { return *(void**)(&object); } diff --git a/strings/base_collections_map.h b/strings/base_collections_map.h index 58d2e0108..66accbe93 100644 --- a/strings/base_collections_map.h +++ b/strings/base_collections_map.h @@ -4,7 +4,7 @@ namespace winrt::impl template using multi_threaded_map = map_impl; - template + template struct observable_map_impl : implements, wfc::IObservableMap, wfc::IMap, wfc::IMapView, wfc::IIterable>>, observable_map_base, K, V, ShouldOriginate>, @@ -32,85 +32,87 @@ namespace winrt::impl Container m_values; }; - template - using observable_map = observable_map_impl; + template + using observable_map = observable_map_impl; - template - using multi_threaded_observable_map = observable_map_impl; + template + using multi_threaded_observable_map = observable_map_impl; } WINRT_EXPORT namespace winrt { - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map() { - return make>>(std::map{}); + return make, ShouldOriginate>>(std::map{}); } - template , typename Allocator = std::allocator>> + + + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map(std::map&& values) { - return make>>(std::move(values)); + return make, ShouldOriginate>>(std::move(values)); } - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map(std::unordered_map&& values) { - return make>>(std::move(values)); + return make, ShouldOriginate>>(std::move(values)); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map() { - return make>>(std::map{}); + return make, ShouldOriginate>>(std::map{}); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map(std::map&& values) { - return make>>(std::move(values)); + return make, ShouldOriginate>>(std::move(values)); } - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map(std::unordered_map&& values) { - return make>>(std::move(values)); + return make, ShouldOriginate>>(std::move(values)); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map() { - return make>>(std::map{}); + return make, ShouldOriginate>>(std::map{}); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::map&& values) { - return make>>(std::move(values)); + return make, ShouldOriginate>>(std::move(values)); } - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::unordered_map&& values) { - return make>>(std::move(values)); + return make, ShouldOriginate>>(std::move(values)); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map() { - return make>>(std::map{}); + return make, ShouldOriginate>>(std::map{}); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::map&& values) { - return make>>(std::move(values)); + return make, ShouldOriginate>>(std::move(values)); } - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::unordered_map&& values) { - return make>>(std::move(values)); + return make, ShouldOriginate>>(std::move(values)); } } diff --git a/strings/base_error.h b/strings/base_error.h index 45d350114..ec3ad0cd8 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -181,6 +181,9 @@ WINRT_EXPORT namespace winrt { struct hresult_error { + struct avoid_originate_t {}; + static constexpr avoid_originate_t avoid_originate{}; + using from_abi_t = take_ownership_from_abi_t; static constexpr auto from_abi{ take_ownership_from_abi }; @@ -206,12 +209,8 @@ WINRT_EXPORT namespace winrt originate(code, nullptr WINRT_IMPL_SOURCE_LOCATION_FORWARD); } - explicit hresult_error(hresult const code, bool avoidOrigination WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) + explicit hresult_error(hresult const code, avoid_originate_t WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) { - if (!avoidOrigination) - { - originate(code, nullptr WINRT_IMPL_SOURCE_LOCATION_FORWARD); - } } hresult_error(hresult const code, param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) @@ -219,14 +218,6 @@ WINRT_EXPORT namespace winrt originate(code, get_abi(message) WINRT_IMPL_SOURCE_LOCATION_FORWARD); } - hresult_error(hresult const code, param::hstring const& message, bool avoidOrigination WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) - { - if (!avoidOrigination) - { - originate(code, get_abi(message) WINRT_IMPL_SOURCE_LOCATION_FORWARD); - } - } - hresult_error(hresult const code, take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : m_code(verify_error(code)) { com_ptr info; @@ -448,14 +439,17 @@ WINRT_EXPORT namespace winrt struct non_originating_hresult_canceled : hresult_error { - non_originating_hresult_canceled(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_canceled, true /*don't originate*/ WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} - non_originating_hresult_canceled(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_canceled, message, true /*don't originate*/ WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_canceled(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_canceled, hresult_error::avoid_originate WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_canceled(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) = delete; + non_originating_hresult_canceled(take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) = delete; + }; struct non_originating_hresult_out_of_bounds : hresult_error { - non_originating_hresult_out_of_bounds(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_out_of_bounds, true /*don't originate*/ WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} - non_originating_hresult_out_of_bounds(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) noexcept : hresult_error(impl::error_out_of_bounds, message, true /*don't originate*/ WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_out_of_bounds(WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM) noexcept : hresult_error(impl::error_out_of_bounds, hresult_error::avoid_originate WINRT_IMPL_SOURCE_LOCATION_FORWARD) {} + non_originating_hresult_out_of_bounds(param::hstring const& message WINRT_IMPL_SOURCE_LOCATION_ARGS) = delete; + non_originating_hresult_out_of_bounds(take_ownership_from_abi_t WINRT_IMPL_SOURCE_LOCATION_ARGS) = delete; }; [[noreturn]] inline WINRT_IMPL_NOINLINE void throw_hresult(hresult const result WINRT_IMPL_SOURCE_LOCATION_ARGS) diff --git a/test/old_tests/UnitTests/Errors.cpp b/test/old_tests/UnitTests/Errors.cpp index 472d633f1..f8656a508 100644 --- a/test/old_tests/UnitTests/Errors.cpp +++ b/test/old_tests/UnitTests/Errors.cpp @@ -233,6 +233,7 @@ TEST_CASE("Errors") // Make sure trimming works. hresult_error e(E_FAIL, L":) is \u263A \n \t "); + auto x = e.message(); REQUIRE(e.message() == L":) is \u263A"); // Make sure delegates propagate correctly. diff --git a/test/old_tests/UnitTests/hresult_error.cpp b/test/old_tests/UnitTests/hresult_error.cpp index fa15c2a1d..d5d118652 100644 --- a/test/old_tests/UnitTests/hresult_error.cpp +++ b/test/old_tests/UnitTests/hresult_error.cpp @@ -494,7 +494,7 @@ TEST_CASE("hresult, throw_last_error") TEST_CASE("hresult, trim_hresult_message") { hresult_error e(E_FAIL, L":) is \u263A \n \t "); - + auto x = e.message(); REQUIRE(e.message() == L":) is \u263A"); } @@ -580,6 +580,7 @@ TEST_CASE("hresult, to_message") } catch (...) { + auto x = to_message(); REQUIRE(to_message() == L"oh no, invalid handle"); } } diff --git a/test/test/async_check_cancel.cpp b/test/test/async_check_cancel.cpp index b9373532c..c29363645 100644 --- a/test/test/async_check_cancel.cpp +++ b/test/test/async_check_cancel.cpp @@ -108,7 +108,6 @@ namespace co_await resume_on_signal(event); auto cancel = co_await get_cancellation_token(); - cancel.avoid_logging(true); if (cancel()) { @@ -133,10 +132,11 @@ namespace REQUIRE(!winrt_throw_hresult_handler); winrt_throw_hresult_handler = exceptionLogger; - co_await resume_on_signal(event); auto cancel = co_await get_cancellation_token(); cancel.avoid_logging(true); + co_await resume_on_signal(event); + if (cancel()) { REQUIRE(!canceled); @@ -172,7 +172,7 @@ namespace async.Cancel(); SetEvent(start.get()); - REQUIRE(WaitForSingleObject(completed.get(), 1000) == WAIT_OBJECT_0); + REQUIRE(WaitForSingleObject(completed.get(), 100000) == WAIT_OBJECT_0); REQUIRE(async.Status() == AsyncStatus::Canceled); REQUIRE(async.ErrorCode() == HRESULT_FROM_WIN32(ERROR_CANCELLED)); @@ -187,10 +187,10 @@ TEST_CASE("async_check_cancel", "[.clang-crash]") TEST_CASE("async_check_cancel") #endif { - //Check(Action); - //Check(ActionWithProgress); - //Check(Operation); - //Check(OperationWithProgress); + Check(Action); + Check(ActionWithProgress); + Check(Operation); + Check(OperationWithProgress); Check(OperationCancelLogged); Check(OperationAvoidLoggingCancel); } diff --git a/test/test_cpp20/custom_error.cpp b/test/test_cpp20/custom_error.cpp index b674c6905..d8eecc058 100644 --- a/test/test_cpp20/custom_error.cpp +++ b/test/test_cpp20/custom_error.cpp @@ -84,7 +84,7 @@ TEST_CASE("custom_error_logger") // now create an empty map that _doesn't_ log s_loggerCalled = false; - IMap c2 = single_threaded_map(std::map{}, true); + IMap c2 = single_threaded_map(std::map{}); val = c2.TryLookup(L"hello"sv); REQUIRE(!val); // ensure that the logger wasn't called From 3972237dd4052d4f62c57c7ec09f382a368237bd Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Wed, 20 Dec 2023 13:09:59 -0800 Subject: [PATCH 06/18] settle on avoid_originate as naming scheme --- strings/base_collections_base.h | 20 ++-- strings/base_collections_input_map.h | 14 +-- strings/base_collections_input_map_view.h | 20 ++-- strings/base_collections_map.h | 140 ++++++++++++++++------ strings/base_coroutine_foundation.h | 11 +- strings/base_coroutine_threadpool.h | 10 +- strings/base_macros.h | 6 +- test/test/async_check_cancel.cpp | 4 +- test/test_cpp20/custom_error.cpp | 55 +++++---- 9 files changed, 179 insertions(+), 101 deletions(-) diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index 861a2602f..437abdaae 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -503,7 +503,7 @@ WINRT_EXPORT namespace winrt }; }; - template + template struct map_view_base : iterable_base, Version> { V Lookup(K const& key) const @@ -513,7 +513,7 @@ WINRT_EXPORT namespace winrt if (pair == static_cast(*this).get_container().end()) { - if constexpr (ShouldOriginate) + if constexpr (avoid_bounds_error_origination) { throw hresult_out_of_bounds(); } @@ -546,8 +546,8 @@ WINRT_EXPORT namespace winrt }; - template - struct map_base : map_view_base + template + struct map_base : map_view_base { Windows::Foundation::Collections::IMapView GetView() const { @@ -579,7 +579,7 @@ WINRT_EXPORT namespace winrt auto found = container.find(static_cast(*this).wrap_value(key)); if (found == container.end()) { - if constexpr (ShouldOriginate) + if constexpr (avoid_bounds_error_origination) { throw hresult_out_of_bounds(); } @@ -602,8 +602,8 @@ WINRT_EXPORT namespace winrt } }; - template - struct observable_map_base : map_base + template + struct observable_map_base : map_base { event_token MapChanged(Windows::Foundation::Collections::MapChangedEventHandler const& handler) { @@ -617,20 +617,20 @@ WINRT_EXPORT namespace winrt bool Insert(K const& key, V const& value) { - bool const result = map_base::Insert(key, value); + bool const result = map_base::Insert(key, value); call_changed(Windows::Foundation::Collections::CollectionChange::ItemInserted, key); return result; } void Remove(K const& key) { - map_base::Remove(key); + map_base::Remove(key); call_changed(Windows::Foundation::Collections::CollectionChange::ItemRemoved, key); } void Clear() noexcept { - map_base::Clear(); + map_base::Clear(); call_changed(Windows::Foundation::Collections::CollectionChange::Reset, impl::empty_value()); } diff --git a/strings/base_collections_input_map.h b/strings/base_collections_input_map.h index 2a17d83d2..e29ca0ccb 100644 --- a/strings/base_collections_input_map.h +++ b/strings/base_collections_input_map.h @@ -1,10 +1,10 @@ namespace winrt::impl { - template + template struct map_impl : - implements, wfc::IMap, wfc::IMapView, wfc::IIterable>>, - map_base, K, V, ShouldOriginate>, + implements, wfc::IMap, wfc::IMapView, wfc::IIterable>>, + map_base, K, V, avoid_bounds_error_origination>, ThreadingBase { static_assert(std::is_same_v>, "Must be constructed with rvalue."); @@ -29,13 +29,13 @@ namespace winrt::impl Container m_values; }; - template - using input_map = map_impl; + template + using input_map = map_impl; - template + template auto make_input_map(Container&& values) { - return make>(std::forward(values)); + return make>(std::forward(values)); } } diff --git a/strings/base_collections_input_map_view.h b/strings/base_collections_input_map_view.h index b7fc9d30f..39e4cf684 100644 --- a/strings/base_collections_input_map_view.h +++ b/strings/base_collections_input_map_view.h @@ -1,10 +1,10 @@ namespace winrt::impl { - template + template struct input_map_view : - implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, - map_view_base, K, V, ShouldOriginate> + implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, + map_view_base, K, V, avoid_bounds_error_origination> { static_assert(std::is_same_v>, "Must be constructed with rvalue."); @@ -22,11 +22,11 @@ namespace winrt::impl Container const m_values; }; - template + template struct scoped_input_map_view : input_scope, - implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, - map_view_base, K, V, ShouldOriginate> + implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, + map_view_base, K, V, avoid_bounds_error_origination> { void abi_enter() const { @@ -53,18 +53,18 @@ namespace winrt::impl Container const& m_values; }; - template + template auto make_input_map_view(Container&& values) { - return make>(std::forward(values)); + return make>(std::forward(values)); } - template + template auto make_scoped_input_map_view(Container const& values) { using interface_type = wfc::IMapView; std::pair result; - auto ptr = new scoped_input_map_view(values); + auto ptr = new scoped_input_map_view(values); *put_abi(result.first) = to_abi(ptr); result.second = ptr; return result; diff --git a/strings/base_collections_map.h b/strings/base_collections_map.h index 66accbe93..97baebb7d 100644 --- a/strings/base_collections_map.h +++ b/strings/base_collections_map.h @@ -1,13 +1,13 @@ namespace winrt::impl { - template - using multi_threaded_map = map_impl; + template + using multi_threaded_map = map_impl; - template + template struct observable_map_impl : - implements, wfc::IObservableMap, wfc::IMap, wfc::IMapView, wfc::IIterable>>, - observable_map_base, K, V, ShouldOriginate>, + implements, wfc::IObservableMap, wfc::IMap, wfc::IMapView, wfc::IIterable>>, + observable_map_base, K, V, avoid_bounds_error_origination>, ThreadingBase { static_assert(std::is_same_v>, "Must be constructed with rvalue."); @@ -32,87 +32,157 @@ namespace winrt::impl Container m_values; }; - template - using observable_map = observable_map_impl; + template + using observable_map = observable_map_impl; - template - using multi_threaded_observable_map = observable_map_impl; + template + using multi_threaded_observable_map = observable_map_impl; } WINRT_EXPORT namespace winrt { - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map() { - return make, ShouldOriginate>>(std::map{}); + return make>>(std::map{}); + } + + template , typename Allocator = std::allocator>> + Windows::Foundation::Collections::IMap single_threaded_map_avoid_originate() + { + return make, true /*avoid_bounds_error_origination*/>>(std::map{}); } - - - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map(std::map&& values) { - return make, ShouldOriginate>>(std::move(values)); + return make>>(std::move(values)); + } + + template , typename Allocator = std::allocator>> + Windows::Foundation::Collections::IMap single_threaded_map_avoid_originate(std::map&& values) + { + return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); } - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map(std::unordered_map&& values) { - return make, ShouldOriginate>>(std::move(values)); + return make>>(std::move(values)); + } + + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + Windows::Foundation::Collections::IMap single_threaded_map_avoid_originate(std::unordered_map&& values) + { + return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map() { - return make, ShouldOriginate>>(std::map{}); + return make>>(std::map{}); + } + + template , typename Allocator = std::allocator>> + Windows::Foundation::Collections::IMap multi_threaded_map_avoid_originate() + { + return make, true /*avoid_bounds_error_origination*/>>(std::map{}); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map(std::map&& values) { - return make, ShouldOriginate>>(std::move(values)); + return make>>(std::move(values)); + } + + template , typename Allocator = std::allocator>> + Windows::Foundation::Collections::IMap multi_threaded_map_avoid_originate(std::map&& values) + { + return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); } - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map(std::unordered_map&& values) { - return make, ShouldOriginate>>(std::move(values)); + return make>>(std::move(values)); + } + + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + Windows::Foundation::Collections::IMap multi_threaded_map_avoid_originate(std::unordered_map&& values) + { + return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map() { - return make, ShouldOriginate>>(std::map{}); + return make>>(std::map{}); + } + + template , typename Allocator = std::allocator>> + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map_avoid_originate() + { + return make, true /*avoid_bounds_error_origination*/>>(std::map{}); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::map&& values) { - return make, ShouldOriginate>>(std::move(values)); + return make>>(std::move(values)); + } + + template , typename Allocator = std::allocator>> + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map_avoid_originate(std::map&& values) + { + return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); } - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::unordered_map&& values) { - return make, ShouldOriginate>>(std::move(values)); + return make>>(std::move(values)); + } + + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + Windows::Foundation::Collections::IObservableMap single_threaded_observable_map_avoid_originate(std::unordered_map&& values) + { + return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map() { - return make, ShouldOriginate>>(std::map{}); + return make>>(std::map{}); + } + + template , typename Allocator = std::allocator>> + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map_avoid_originate() + { + return make, true /*avoid_bounds_error_origination*/>>(std::map{}); } - template , typename Allocator = std::allocator>> + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::map&& values) { - return make, ShouldOriginate>>(std::move(values)); + return make>>(std::move(values)); + } + + template , typename Allocator = std::allocator>> + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map_avoid_originate(std::map&& values) + { + return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); } - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::unordered_map&& values) { - return make, ShouldOriginate>>(std::move(values)); + return make>>(std::move(values)); + } + + template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> + Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map_avoid_originate(std::unordered_map&& values) + { + return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); } } diff --git a/strings/base_coroutine_foundation.h b/strings/base_coroutine_foundation.h index 6d9c41e4f..9e7ce581a 100644 --- a/strings/base_coroutine_foundation.h +++ b/strings/base_coroutine_foundation.h @@ -202,11 +202,10 @@ namespace winrt::impl static fire_and_forget cancel_asynchronously(Async async) { - auto asyncRef = async; co_await winrt::resume_background(); try { - asyncRef.Cancel(); + async.Cancel(); } catch (hresult_error const&) { @@ -352,9 +351,9 @@ namespace winrt::impl return m_promise->enable_cancellation_propagation(value); } - bool avoid_logging(bool value = true) const noexcept + bool avoid_cancel_origination(bool value = true) const noexcept { - return m_promise->enable_avoid_rooriginate_cancel(value); + return m_promise->enable_avoid_cancel_origination(value); } private: @@ -489,7 +488,7 @@ namespace winrt::impl if (m_status.load(std::memory_order_relaxed) == AsyncStatus::Started) { m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed); - if (cancellable_promise::avoid_rooriginate_cancel_enabled()) + if (cancellable_promise::avoid_cancel_origination_enabled()) { m_exception = std::make_exception_ptr(non_originating_hresult_canceled()); } @@ -644,7 +643,7 @@ namespace winrt::impl { if (Status() == AsyncStatus::Canceled) { - if (cancellable_promise::avoid_rooriginate_cancel_enabled()) + if (cancellable_promise::avoid_cancel_origination_enabled()) { throw winrt::non_originating_hresult_canceled(); } diff --git a/strings/base_coroutine_threadpool.h b/strings/base_coroutine_threadpool.h index 117af8d59..ba7bfe3a1 100644 --- a/strings/base_coroutine_threadpool.h +++ b/strings/base_coroutine_threadpool.h @@ -180,14 +180,14 @@ WINRT_EXPORT namespace winrt return m_propagate_cancellation; } - bool enable_avoid_rooriginate_cancel(bool value) noexcept + bool enable_avoid_cancel_origination(bool value) noexcept { - return std::exchange(m_avoid_rooriginate_cancel, value); + return std::exchange(m_avoid_cancel_origination, value); } - bool avoid_rooriginate_cancel_enabled() const noexcept + bool avoid_cancel_origination_enabled() const noexcept { - return m_avoid_rooriginate_cancel; + return m_avoid_cancel_origination; } private: @@ -196,7 +196,7 @@ WINRT_EXPORT namespace winrt std::atomic m_canceller{ nullptr }; void* m_context{ nullptr }; bool m_propagate_cancellation{ false }; - bool m_avoid_rooriginate_cancel{ false }; + bool m_avoid_cancel_origination{ false }; }; template diff --git a/strings/base_macros.h b/strings/base_macros.h index deea7e7c1..457ae7a3d 100644 --- a/strings/base_macros.h +++ b/strings/base_macros.h @@ -85,9 +85,9 @@ typedef struct _GUID GUID; // Some projects may decide to disable std::source_location support to prevent source code information from ending up in their // release binaries, or to reduce binary size. Defining WINRT_NO_SOURCE_LOCATION will prevent this feature from activating. #if defined(__cpp_lib_source_location) && !defined(WINRT_NO_SOURCE_LOCATION) -#define WINRT_IMPL_SOURCE_LOCATION_ARGS_NO_DEFAULT , std::source_location const& sourceInformation -#define WINRT_IMPL_SOURCE_LOCATION_ARGS , std::source_location const& sourceInformation = std::source_location::current() -#define WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM std::source_location const& sourceInformation = std::source_location::current() +#define WINRT_IMPL_SOURCE_LOCATION_ARGS_NO_DEFAULT , [[maybe_unused]] std::source_location const& sourceInformation +#define WINRT_IMPL_SOURCE_LOCATION_ARGS , [[maybe_unused]]std::source_location const& sourceInformation = std::source_location::current() +#define WINRT_IMPL_SOURCE_LOCATION_ARGS_SINGLE_PARAM [[maybe_unused]] std::source_location const& sourceInformation = std::source_location::current() #define WINRT_IMPL_SOURCE_LOCATION_FORWARD , sourceInformation #define WINRT_IMPL_SOURCE_LOCATION_FORWARD_SINGLE_PARAM sourceInformation diff --git a/test/test/async_check_cancel.cpp b/test/test/async_check_cancel.cpp index c29363645..11c902c71 100644 --- a/test/test/async_check_cancel.cpp +++ b/test/test/async_check_cancel.cpp @@ -133,7 +133,7 @@ namespace winrt_throw_hresult_handler = exceptionLogger; auto cancel = co_await get_cancellation_token(); - cancel.avoid_logging(true); + cancel.avoid_cancel_origination(true); co_await resume_on_signal(event); @@ -172,7 +172,7 @@ namespace async.Cancel(); SetEvent(start.get()); - REQUIRE(WaitForSingleObject(completed.get(), 100000) == WAIT_OBJECT_0); + REQUIRE(WaitForSingleObject(completed.get(), IsDebuggerPresent() ? INFINITE : 1000) == WAIT_OBJECT_0); REQUIRE(async.Status() == AsyncStatus::Canceled); REQUIRE(async.ErrorCode() == HRESULT_FROM_WIN32(ERROR_CANCELLED)); diff --git a/test/test_cpp20/custom_error.cpp b/test/test_cpp20/custom_error.cpp index d8eecc058..389222f2d 100644 --- a/test/test_cpp20/custom_error.cpp +++ b/test/test_cpp20/custom_error.cpp @@ -64,34 +64,43 @@ TEST_CASE("custom_error_logger") REQUIRE(s_loggerArgs.returnAddress); REQUIRE(s_loggerArgs.result == static_cast(0x80000018)); // E_ILLEGAL_DELEGATE_ASSIGNMENT) +} - s_loggerCalled = false; +#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION < 170000 +// not available in libc++ before LLVM 16 +TEST_CASE("custom_error_logger_with_avoid_origination", "[!shouldfail]") +#else +TEST_CASE("custom_error_logger_with_avoid_origination") +#endif +{ + // Set up global handler + REQUIRE(!s_loggerCalled); + REQUIRE(!winrt_throw_hresult_handler); + winrt_throw_hresult_handler = logger; - { - // Log a try lookup - IMap c = single_threaded_map(); - c.Insert(L"hello"sv, 1); - c.Lookup(L"hello"sv); - auto val = c.TryLookup(L"hello"sv); - REQUIRE(val); - c.Remove(L"hello"sv); - REQUIRE(!s_loggerCalled); - // Now after we remove the item, TryLookup will fail and the logger will be called. + // Log a try lookup + IMap c = single_threaded_map(); + c.Insert(L"hello"sv, 1); + c.Lookup(L"hello"sv); + auto val = c.TryLookup(L"hello"sv); + REQUIRE(val); + c.Remove(L"hello"sv); + REQUIRE(!s_loggerCalled); + // Now after we remove the item, TryLookup will fail and the logger will be called. - val = c.TryLookup(L"hello"sv); - REQUIRE(!val); - REQUIRE(s_loggerCalled); + val = c.TryLookup(L"hello"sv); + REQUIRE(!val); + REQUIRE(s_loggerCalled); - // now create an empty map that _doesn't_ log - s_loggerCalled = false; - IMap c2 = single_threaded_map(std::map{}); - val = c2.TryLookup(L"hello"sv); - REQUIRE(!val); - // ensure that the logger wasn't called - REQUIRE(!s_loggerCalled); - } + // now create an empty map that _doesn't_ log + s_loggerCalled = false; + IMap c2 = single_threaded_map_avoid_originate(std::map{}); + val = c2.TryLookup(L"hello"sv); + REQUIRE(!val); + // ensure that the logger wasn't called + REQUIRE(!s_loggerCalled); // Remove global handler winrt_throw_hresult_handler = nullptr; s_loggerCalled = false; -} +} \ No newline at end of file From dd18558bbd253ca6939c120cc4de49a9aaa33475 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Mon, 15 Sep 2025 17:23:02 -0700 Subject: [PATCH 07/18] add a printf only to Lookup so we can add SFINAE or something like that. --- cppwinrt/code_writers.h | 63 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index ffc0ee948..078844ff9 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1873,6 +1873,36 @@ namespace cppwinrt } } + static void write_produce_upcall_LOOKUP(writer& w, std::string_view const& upcall, method_signature const& method_signature) + { + if (method_signature.return_signature()) + { + auto name = method_signature.return_param_name(); + + w.write("*% = detach_from<%>(%(%));", + name, + method_signature.return_signature(), + upcall, + bind(method_signature)); + } + else + { + w.write("%(%);", + upcall, + bind(method_signature)); + } + + for (auto&& [param, param_signature] : method_signature.params()) + { + if (param.Flags().Out() && !param_signature->Type().is_szarray() && is_object(param_signature->Type())) + { + auto param_name = param.Name(); + + w.write("\n if (%) *% = detach_abi(winrt_impl_%);", param_name, param_name, param_name); + } + } + } + static void write_produce_method(writer& w, MethodDef const& method) { std::string_view format; @@ -1902,13 +1932,32 @@ namespace cppwinrt method_signature signature{ method }; auto async_types_guard = w.push_async_types(signature.is_async()); std::string upcall = "this->shim()."; - upcall += get_name(method); - - w.write(format, - get_abi_name(method), - bind(signature), - bind(signature), - bind(upcall, signature)); + auto name = get_name(method); + upcall += name; + if (name == "Lookup") + { + format = R"( int32_t __stdcall %(%) noexcept final try + { +% typename D::abi_guard guard(this->shim());// hello world + % + return 0; + } + catch (...) { return to_hresult(); } +)"; + w.write(format, + get_abi_name(method), + bind(signature), + bind(signature), + bind(upcall, signature)); + } + else + { + w.write(format, + get_abi_name(method), + bind(signature), + bind(signature), + bind(upcall, signature)); + } } static void write_fast_produce_methods(writer& w, TypeDef const& default_interface) From 0dfa235d519c4020c5624669c6f380a20738ef52 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Mon, 15 Sep 2025 18:48:48 -0700 Subject: [PATCH 08/18] weird templating magic required... still not working, has error 1>G:\source\repos\cppwinrt\_build\x64\Debug\winrt\Windows.Foundation.Collections.h(868,51): error C3878: syntax error: unexpected token '>' following 'simple-type-specifier' 1>(compiling source file '/Class.cpp') 1> G:\source\repos\cppwinrt\_build\x64\Debug\winrt\Windows.Foundation.Collections.h(868,51): 1> missing one of: '(' '{' ? --- cppwinrt/code_writers.h | 37 ++++++++++++++++++++++----------- strings/base_collections.h | 21 +++++++++++++++++++ strings/base_collections_base.h | 12 +++++++++++ 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 078844ff9..7296ea3d6 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1875,21 +1875,26 @@ namespace cppwinrt static void write_produce_upcall_LOOKUP(writer& w, std::string_view const& upcall, method_signature const& method_signature) { - if (method_signature.return_signature()) + // assert if (method_signature.return_signature()) { auto name = method_signature.return_param_name(); - w.write("*% = detach_from<%>(%(%));", - name, + w.write("auto out_param_val = detach_from<%>(%(%));", + //name, method_signature.return_signature(), upcall, bind(method_signature)); - } - else - { - w.write("%(%);", - upcall, - bind(method_signature)); + w.write(R"( + if (out_param_val) + { + *% = out_param_val; + } + else + { + return impl::error_out_of_bounds; + } +)", + name); } for (auto&& [param, param_signature] : method_signature.params()) @@ -1939,7 +1944,14 @@ namespace cppwinrt format = R"( int32_t __stdcall %(%) noexcept final try { % typename D::abi_guard guard(this->shim());// hello world - % + if constexpr (impl::has_try_lookup_v) + { + % + } + else + { + % + } return 0; } catch (...) { return to_hresult(); } @@ -1947,8 +1959,9 @@ namespace cppwinrt w.write(format, get_abi_name(method), bind(signature), - bind(signature), - bind(upcall, signature)); + bind(signature), // clear_abi + bind(upcall, signature), + bind(upcall, signature)); } else { diff --git a/strings/base_collections.h b/strings/base_collections.h index cba0864b3..96b4d414b 100644 --- a/strings/base_collections.h +++ b/strings/base_collections.h @@ -48,6 +48,27 @@ namespace winrt::impl template struct is_key_value_pair> : std::true_type {}; + + struct TryLookupable : impl::marker {}; + template > + struct has_try_lookup : std::false_type {}; + + template + struct has_try_lookup : std::true_type{}; + + template + inline constexpr bool has_try_lookup_v = has_try_lookup::value; + + //template class component_has_TryLookup { + // template static std::false_type test(...); + // template static auto test(int) + // -> decltype(std::declval().TryLookup( + // std::declval()), std::true_type()); + //public: + // static constexpr bool value + // = std::is_same(0)), std::true_type>::value; + //}; + struct input_scope { void invalidate_scope() noexcept diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index 038078635..d08e55e7c 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -506,6 +506,18 @@ WINRT_EXPORT namespace winrt template struct map_view_base : iterable_base, Version> { + V TryLookup(K const& key) const + { + [[maybe_unused]] auto guard = static_cast(*this).acquire_shared(); + auto pair = static_cast(*this).get_container().find(static_cast(*this).wrap_value(key)); + + if (pair == static_cast(*this).get_container().end()) + { + return nullptr; + } + + return static_cast(*this).unwrap_value(pair->second); + } V Lookup(K const& key) const { [[maybe_unused]] auto guard = static_cast(*this).acquire_shared(); From 3f448142889800773b079a05138b2a3fa3ba1b7c Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Tue, 16 Sep 2025 18:28:58 -0700 Subject: [PATCH 09/18] trylookup exists to avoid throwing an error, and let's avoid avoid_originate now. --- .runsettings | 8 +- cppwinrt/code_writers.h | 13 ++-- strings/base_collections.h | 21 ------ strings/base_collections_base.h | 44 ++++------- strings/base_collections_input_map.h | 14 ++-- strings/base_collections_input_map_view.h | 20 ++--- strings/base_collections_map.h | 90 +++-------------------- strings/base_error.h | 7 -- strings/base_meta.h | 9 +++ 9 files changed, 61 insertions(+), 165 deletions(-) diff --git a/.runsettings b/.runsettings index 2c3e15574..2ee4631fc 100644 --- a/.runsettings +++ b/.runsettings @@ -10,14 +10,14 @@ - Verbose - ^test + Single + ^test true 30000 on true - Normal - StatsOnly + Verbose + AdditionalInfo ShortInfo , 20000 diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 7296ea3d6..190e89895 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1879,22 +1879,20 @@ namespace cppwinrt { auto name = method_signature.return_param_name(); - w.write("auto out_param_val = detach_from<%>(%(%));", - //name, - method_signature.return_signature(), + w.write("auto out_param_val = %(%);", upcall, bind(method_signature)); w.write(R"( if (out_param_val) { - *% = out_param_val; + *% = detach_from<%>(std::move(*out_param_val)); } else { return impl::error_out_of_bounds; } )", - name); + name, method_signature.return_signature()); } for (auto&& [param, param_signature] : method_signature.params()) @@ -1941,10 +1939,11 @@ namespace cppwinrt upcall += name; if (name == "Lookup") { + std::string tryLookupUpCall = "this->shim().TryLookup"; format = R"( int32_t __stdcall %(%) noexcept final try { % typename D::abi_guard guard(this->shim());// hello world - if constexpr (impl::has_try_lookup_v) + if constexpr (has_try_lookup_v) { % } @@ -1960,7 +1959,7 @@ namespace cppwinrt get_abi_name(method), bind(signature), bind(signature), // clear_abi - bind(upcall, signature), + bind(tryLookupUpCall, signature), bind(upcall, signature)); } else diff --git a/strings/base_collections.h b/strings/base_collections.h index 96b4d414b..cba0864b3 100644 --- a/strings/base_collections.h +++ b/strings/base_collections.h @@ -48,27 +48,6 @@ namespace winrt::impl template struct is_key_value_pair> : std::true_type {}; - - struct TryLookupable : impl::marker {}; - template > - struct has_try_lookup : std::false_type {}; - - template - struct has_try_lookup : std::true_type{}; - - template - inline constexpr bool has_try_lookup_v = has_try_lookup::value; - - //template class component_has_TryLookup { - // template static std::false_type test(...); - // template static auto test(int) - // -> decltype(std::declval().TryLookup( - // std::declval()), std::true_type()); - //public: - // static constexpr bool value - // = std::is_same(0)), std::true_type>::value; - //}; - struct input_scope { void invalidate_scope() noexcept diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index d08e55e7c..bc756f078 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -503,21 +503,23 @@ WINRT_EXPORT namespace winrt }; }; - template + template struct map_view_base : iterable_base, Version> { - V TryLookup(K const& key) const + // specialization of Lookup that avoids throwing the hresult + std::optional TryLookup(K const& key) const { [[maybe_unused]] auto guard = static_cast(*this).acquire_shared(); auto pair = static_cast(*this).get_container().find(static_cast(*this).wrap_value(key)); if (pair == static_cast(*this).get_container().end()) { - return nullptr; + return { std::nullopt }; } - return static_cast(*this).unwrap_value(pair->second); + return { static_cast(*this).unwrap_value(pair->second) }; } + V Lookup(K const& key) const { [[maybe_unused]] auto guard = static_cast(*this).acquire_shared(); @@ -525,14 +527,7 @@ WINRT_EXPORT namespace winrt if (pair == static_cast(*this).get_container().end()) { - if constexpr (avoid_bounds_error_origination) - { - throw non_originating_hresult_out_of_bounds(); - } - else - { - throw hresult_out_of_bounds(); - } + throw hresult_out_of_bounds(); } return static_cast(*this).unwrap_value(pair->second); @@ -558,8 +553,8 @@ WINRT_EXPORT namespace winrt }; - template - struct map_base : map_view_base + template + struct map_base : map_view_base { Windows::Foundation::Collections::IMapView GetView() const { @@ -581,7 +576,7 @@ WINRT_EXPORT namespace winrt return !added; } - + // todo also create a tryremove? void Remove(K const& key) { typename impl::container_type_t::node_type removedNode; @@ -591,14 +586,7 @@ WINRT_EXPORT namespace winrt auto found = container.find(static_cast(*this).wrap_value(key)); if (found == container.end()) { - if constexpr (avoid_bounds_error_origination) - { - throw hresult_out_of_bounds(); - } - else - { - throw non_originating_hresult_out_of_bounds(); - } + throw hresult_out_of_bounds(); } this->increment_version(); removedNode = container.extract(found); @@ -614,8 +602,8 @@ WINRT_EXPORT namespace winrt } }; - template - struct observable_map_base : map_base + template + struct observable_map_base : map_base { event_token MapChanged(Windows::Foundation::Collections::MapChangedEventHandler const& handler) { @@ -629,20 +617,20 @@ WINRT_EXPORT namespace winrt bool Insert(K const& key, V const& value) { - bool const result = map_base::Insert(key, value); + bool const result = map_base::Insert(key, value); call_changed(Windows::Foundation::Collections::CollectionChange::ItemInserted, key); return result; } void Remove(K const& key) { - map_base::Remove(key); + map_base::Remove(key); call_changed(Windows::Foundation::Collections::CollectionChange::ItemRemoved, key); } void Clear() noexcept { - map_base::Clear(); + map_base::Clear(); call_changed(Windows::Foundation::Collections::CollectionChange::Reset, impl::empty_value()); } diff --git a/strings/base_collections_input_map.h b/strings/base_collections_input_map.h index e29ca0ccb..8bf36e3c8 100644 --- a/strings/base_collections_input_map.h +++ b/strings/base_collections_input_map.h @@ -1,10 +1,10 @@ namespace winrt::impl { - template + template struct map_impl : - implements, wfc::IMap, wfc::IMapView, wfc::IIterable>>, - map_base, K, V, avoid_bounds_error_origination>, + implements, wfc::IMap, wfc::IMapView, wfc::IIterable>>, + map_base, K, V>, ThreadingBase { static_assert(std::is_same_v>, "Must be constructed with rvalue."); @@ -29,13 +29,13 @@ namespace winrt::impl Container m_values; }; - template - using input_map = map_impl; + template + using input_map = map_impl; - template + template auto make_input_map(Container&& values) { - return make>(std::forward(values)); + return make>(std::forward(values)); } } diff --git a/strings/base_collections_input_map_view.h b/strings/base_collections_input_map_view.h index 39e4cf684..bfd8d82a9 100644 --- a/strings/base_collections_input_map_view.h +++ b/strings/base_collections_input_map_view.h @@ -1,10 +1,10 @@ namespace winrt::impl { - template + template struct input_map_view : - implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, - map_view_base, K, V, avoid_bounds_error_origination> + implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, + map_view_base, K, V> { static_assert(std::is_same_v>, "Must be constructed with rvalue."); @@ -22,11 +22,11 @@ namespace winrt::impl Container const m_values; }; - template + template struct scoped_input_map_view : input_scope, - implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, - map_view_base, K, V, avoid_bounds_error_origination> + implements, non_agile, no_weak_ref, wfc::IMapView, wfc::IIterable>>, + map_view_base, K, V> { void abi_enter() const { @@ -53,18 +53,18 @@ namespace winrt::impl Container const& m_values; }; - template + template auto make_input_map_view(Container&& values) { - return make>(std::forward(values)); + return make>(std::forward(values)); } - template + template auto make_scoped_input_map_view(Container const& values) { using interface_type = wfc::IMapView; std::pair result; - auto ptr = new scoped_input_map_view(values); + auto ptr = new scoped_input_map_view(values); *put_abi(result.first) = to_abi(ptr); result.second = ptr; return result; diff --git a/strings/base_collections_map.h b/strings/base_collections_map.h index 97baebb7d..64ab80ff7 100644 --- a/strings/base_collections_map.h +++ b/strings/base_collections_map.h @@ -1,13 +1,13 @@ namespace winrt::impl { - template - using multi_threaded_map = map_impl; + template + using multi_threaded_map = map_impl; - template + template struct observable_map_impl : - implements, wfc::IObservableMap, wfc::IMap, wfc::IMapView, wfc::IIterable>>, - observable_map_base, K, V, avoid_bounds_error_origination>, + implements, wfc::IObservableMap, wfc::IMap, wfc::IMapView, wfc::IIterable>>, + observable_map_base, K, V>, ThreadingBase { static_assert(std::is_same_v>, "Must be constructed with rvalue."); @@ -32,11 +32,11 @@ namespace winrt::impl Container m_values; }; - template - using observable_map = observable_map_impl; + template + using observable_map = observable_map_impl; - template - using multi_threaded_observable_map = observable_map_impl; + template + using multi_threaded_observable_map = observable_map_impl; } WINRT_EXPORT namespace winrt @@ -47,143 +47,71 @@ WINRT_EXPORT namespace winrt return make>>(std::map{}); } - template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map_avoid_originate() - { - return make, true /*avoid_bounds_error_origination*/>>(std::map{}); - } - template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map(std::map&& values) { return make>>(std::move(values)); } - - template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map_avoid_originate(std::map&& values) - { - return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); - } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map(std::unordered_map&& values) { return make>>(std::move(values)); } - - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap single_threaded_map_avoid_originate(std::unordered_map&& values) - { - return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); - } template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map() { return make>>(std::map{}); } - - template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map_avoid_originate() - { - return make, true /*avoid_bounds_error_origination*/>>(std::map{}); - } template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map(std::map&& values) { return make>>(std::move(values)); } - - template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map_avoid_originate(std::map&& values) - { - return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); - } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap multi_threaded_map(std::unordered_map&& values) { return make>>(std::move(values)); } - - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IMap multi_threaded_map_avoid_originate(std::unordered_map&& values) - { - return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); - } template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map() { return make>>(std::map{}); } - - template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map_avoid_originate() - { - return make, true /*avoid_bounds_error_origination*/>>(std::map{}); - } template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::map&& values) { return make>>(std::move(values)); } - - template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map_avoid_originate(std::map&& values) - { - return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); - } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap single_threaded_observable_map(std::unordered_map&& values) { return make>>(std::move(values)); } - - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap single_threaded_observable_map_avoid_originate(std::unordered_map&& values) - { - return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); - } template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map() { return make>>(std::map{}); } - - template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map_avoid_originate() - { - return make, true /*avoid_bounds_error_origination*/>>(std::map{}); - } template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::map&& values) { return make>>(std::move(values)); } - - template , typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map_avoid_originate(std::map&& values) - { - return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); - } template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map(std::unordered_map&& values) { return make>>(std::move(values)); } - - template , typename KeyEqual = std::equal_to, typename Allocator = std::allocator>> - Windows::Foundation::Collections::IObservableMap multi_threaded_observable_map_avoid_originate(std::unordered_map&& values) - { - return make, true /*avoid_bounds_error_origination*/>>(std::move(values)); - } } namespace std diff --git a/strings/base_error.h b/strings/base_error.h index 36a858276..e4535f4c4 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -344,13 +344,6 @@ WINRT_EXPORT namespace winrt }; - struct non_originating_hresult_out_of_bounds : hresult_error - { - non_originating_hresult_out_of_bounds(winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_out_of_bounds, hresult_error::avoid_originate, sourceInformation) {} - non_originating_hresult_out_of_bounds(param::hstring const& message, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) = delete; - non_originating_hresult_out_of_bounds(take_ownership_from_abi_t, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) = delete; - }; - [[noreturn]] inline WINRT_IMPL_NOINLINE void throw_hresult(hresult const result, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) { if (winrt_throw_hresult_handler) diff --git a/strings/base_meta.h b/strings/base_meta.h index 25deb42ec..76231d7d8 100644 --- a/strings/base_meta.h +++ b/strings/base_meta.h @@ -298,4 +298,13 @@ namespace winrt::impl return (func(Types{}) || ...); } }; + + template > + struct has_try_lookup : std::false_type {}; + + template + struct has_try_lookup : std::true_type{}; + + template + inline constexpr bool has_try_lookup_v = has_try_lookup::value; } From cfaa2a6de3483b499e29a5ee898e7f1549555e40 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Wed, 17 Sep 2025 13:04:17 -0700 Subject: [PATCH 10/18] remove test. --- test/test_cpp20/custom_error.cpp | 39 -------------------------------- 1 file changed, 39 deletions(-) diff --git a/test/test_cpp20/custom_error.cpp b/test/test_cpp20/custom_error.cpp index 2caa73129..2bde6d72e 100644 --- a/test/test_cpp20/custom_error.cpp +++ b/test/test_cpp20/custom_error.cpp @@ -73,42 +73,3 @@ TEST_CASE("custom_error_logger") winrt_throw_hresult_handler = nullptr; s_loggerCalled = false; } - -#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION < 170000 -// not available in libc++ before LLVM 16 -TEST_CASE("custom_error_logger_with_avoid_origination", "[!shouldfail]") -#else -TEST_CASE("custom_error_logger_with_avoid_origination") -#endif -{ - // Set up global handler - REQUIRE(!s_loggerCalled); - REQUIRE(!winrt_throw_hresult_handler); - winrt_throw_hresult_handler = logger; - - // Log a try lookup - IMap c = single_threaded_map(); - c.Insert(L"hello"sv, 1); - c.Lookup(L"hello"sv); - auto val = c.TryLookup(L"hello"sv); - REQUIRE(val); - c.Remove(L"hello"sv); - REQUIRE(!s_loggerCalled); - // Now after we remove the item, TryLookup will fail and the logger will be called. - - val = c.TryLookup(L"hello"sv); - REQUIRE(!val); - REQUIRE(s_loggerCalled); - - // now create an empty map that _doesn't_ log - s_loggerCalled = false; - IMap c2 = single_threaded_map_avoid_originate(std::map{}); - val = c2.TryLookup(L"hello"sv); - REQUIRE(!val); - // ensure that the logger wasn't called - REQUIRE(!s_loggerCalled); - - // Remove global handler - winrt_throw_hresult_handler = nullptr; - s_loggerCalled = false; -} \ No newline at end of file From e69a7fe1dea8649d3bacfc08fe8413b00098e302 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Wed, 17 Sep 2025 16:19:54 -0700 Subject: [PATCH 11/18] cleaned up and correctness. --- .runsettings | 6 +++--- cppwinrt/code_writers.h | 41 ++++++++++++++++++++--------------------- strings/base_meta.h | 17 +++++++++++------ 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/.runsettings b/.runsettings index 2ee4631fc..075c5e97c 100644 --- a/.runsettings +++ b/.runsettings @@ -10,13 +10,13 @@ - Single - ^test + Single + (?i:Test) true 30000 on true - Verbose + Verbose AdditionalInfo ShortInfo , diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 190e89895..17acad694 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1873,27 +1873,24 @@ namespace cppwinrt } } - static void write_produce_upcall_LOOKUP(writer& w, std::string_view const& upcall, method_signature const& method_signature) + static void write_produce_upcall_TryLookup(writer& w, std::string_view const& upcall, method_signature const& method_signature) { - // assert if (method_signature.return_signature()) - { - auto name = method_signature.return_param_name(); + auto name = method_signature.return_param_name(); - w.write("auto out_param_val = %(%);", - upcall, - bind(method_signature)); - w.write(R"( - if (out_param_val) - { - *% = detach_from<%>(std::move(*out_param_val)); - } - else - { - return impl::error_out_of_bounds; - } + w.write("auto out_param_val = %(%);", + upcall, + bind(method_signature)); + w.write(R"( + if (out_param_val) + { + *% = detach_from<%>(std::move(*out_param_val)); + } + else + { + return impl::error_out_of_bounds; + } )", - name, method_signature.return_signature()); - } + name, method_signature.return_signature()); for (auto&& [param, param_signature] : method_signature.params()) { @@ -1939,11 +1936,13 @@ namespace cppwinrt upcall += name; if (name == "Lookup") { + // Special-case Lookup to look for a TryLookup here, to avoid a throw/originate + // and add a small performance improvement. std::string tryLookupUpCall = "this->shim().TryLookup"; format = R"( int32_t __stdcall %(%) noexcept final try { -% typename D::abi_guard guard(this->shim());// hello world - if constexpr (has_try_lookup_v) +% typename D::abi_guard guard(this->shim()); + if constexpr (has_try_lookup_v) { % } @@ -1959,7 +1958,7 @@ namespace cppwinrt get_abi_name(method), bind(signature), bind(signature), // clear_abi - bind(tryLookupUpCall, signature), + bind(tryLookupUpCall, signature), bind(upcall, signature)); } else diff --git a/strings/base_meta.h b/strings/base_meta.h index 76231d7d8..1cc3edd70 100644 --- a/strings/base_meta.h +++ b/strings/base_meta.h @@ -299,12 +299,17 @@ namespace winrt::impl } }; - template > - struct has_try_lookup : std::false_type {}; + template + struct has_try_lookup + { + template ().TryLookup(K{}))> static constexpr bool get_value(int) { return true; } + template static constexpr bool get_value(...) { return false; } - template - struct has_try_lookup : std::true_type{}; + public: - template - inline constexpr bool has_try_lookup_v = has_try_lookup::value; + static constexpr bool value = get_value(0); + }; + + template + inline constexpr bool has_try_lookup_v = has_try_lookup::value; } From 7051f24ffbf8e8bd9b58583c0cf564258b86157e Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Wed, 17 Sep 2025 16:30:41 -0700 Subject: [PATCH 12/18] undo changes to untouched files --- strings/base_collections_input_map.h | 4 +++- strings/base_collections_map.h | 6 ++++-- test/old_tests/UnitTests/hresult_error.cpp | 3 +-- test/test_cpp20/custom_error.cpp | 1 - 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/strings/base_collections_input_map.h b/strings/base_collections_input_map.h index 8bf36e3c8..b33975fe6 100644 --- a/strings/base_collections_input_map.h +++ b/strings/base_collections_input_map.h @@ -9,7 +9,9 @@ namespace winrt::impl { static_assert(std::is_same_v>, "Must be constructed with rvalue."); - explicit map_impl(Container&& values) : m_values(std::forward(values)) {} + explicit map_impl(Container&& values) : m_values(std::forward(values)) + { + } auto& get_container() noexcept { diff --git a/strings/base_collections_map.h b/strings/base_collections_map.h index 64ab80ff7..f4bd74baf 100644 --- a/strings/base_collections_map.h +++ b/strings/base_collections_map.h @@ -12,7 +12,9 @@ namespace winrt::impl { static_assert(std::is_same_v>, "Must be constructed with rvalue."); - explicit observable_map_impl(Container&& values) : m_values(std::forward(values)) {} + explicit observable_map_impl(Container&& values) : m_values(std::forward(values)) + { + } auto& get_container() noexcept { @@ -46,7 +48,7 @@ WINRT_EXPORT namespace winrt { return make>>(std::map{}); } - + template , typename Allocator = std::allocator>> Windows::Foundation::Collections::IMap single_threaded_map(std::map&& values) { diff --git a/test/old_tests/UnitTests/hresult_error.cpp b/test/old_tests/UnitTests/hresult_error.cpp index d5d118652..fa15c2a1d 100644 --- a/test/old_tests/UnitTests/hresult_error.cpp +++ b/test/old_tests/UnitTests/hresult_error.cpp @@ -494,7 +494,7 @@ TEST_CASE("hresult, throw_last_error") TEST_CASE("hresult, trim_hresult_message") { hresult_error e(E_FAIL, L":) is \u263A \n \t "); - auto x = e.message(); + REQUIRE(e.message() == L":) is \u263A"); } @@ -580,7 +580,6 @@ TEST_CASE("hresult, to_message") } catch (...) { - auto x = to_message(); REQUIRE(to_message() == L"oh no, invalid handle"); } } diff --git a/test/test_cpp20/custom_error.cpp b/test/test_cpp20/custom_error.cpp index 2bde6d72e..d7b055e47 100644 --- a/test/test_cpp20/custom_error.cpp +++ b/test/test_cpp20/custom_error.cpp @@ -2,7 +2,6 @@ using namespace winrt; using namespace Windows::Foundation; -using namespace Windows::Foundation::Collections; namespace { From 7b0072a124a3cd81eebd5327e98bc2d640c1c3f7 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Thu, 18 Sep 2025 14:29:52 -0700 Subject: [PATCH 13/18] spacing changes , change do declval. --- cppwinrt/code_writers.h | 16 ++++++++-------- strings/base_collections_base.h | 2 +- strings/base_error.h | 1 - strings/base_meta.h | 2 +- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 17acad694..9f098b4d8 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1881,14 +1881,14 @@ namespace cppwinrt upcall, bind(method_signature)); w.write(R"( - if (out_param_val) - { - *% = detach_from<%>(std::move(*out_param_val)); - } - else - { - return impl::error_out_of_bounds; - } + if (out_param_val) + { + *% = detach_from<%>(std::move(*out_param_val)); + } + else + { + return impl::error_out_of_bounds; + } )", name, method_signature.return_signature()); diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index bc756f078..0f94266a1 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -576,7 +576,7 @@ WINRT_EXPORT namespace winrt return !added; } - // todo also create a tryremove? + void Remove(K const& key) { typename impl::container_type_t::node_type removedNode; diff --git a/strings/base_error.h b/strings/base_error.h index e4535f4c4..2e90b66da 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -341,7 +341,6 @@ WINRT_EXPORT namespace winrt non_originating_hresult_canceled(winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, hresult_error::avoid_originate, sourceInformation) {} non_originating_hresult_canceled(param::hstring const& message, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) = delete; non_originating_hresult_canceled(take_ownership_from_abi_t , winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) = delete; - }; [[noreturn]] inline WINRT_IMPL_NOINLINE void throw_hresult(hresult const result, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) diff --git a/strings/base_meta.h b/strings/base_meta.h index 1cc3edd70..d30f9cefc 100644 --- a/strings/base_meta.h +++ b/strings/base_meta.h @@ -302,7 +302,7 @@ namespace winrt::impl template struct has_try_lookup { - template ().TryLookup(K{}))> static constexpr bool get_value(int) { return true; } + template ().TryLookup(std::declval()))> static constexpr bool get_value(int) { return true; } template static constexpr bool get_value(...) { return false; } public: From 783413a8cbb6bf73a7625d6ca422097472824e5c Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Thu, 18 Sep 2025 18:27:39 -0700 Subject: [PATCH 14/18] added special tag to parameter list to make sure not to break existing TryLookup impls, added test to verify this. --- cppwinrt/code_writers.h | 4 +-- strings/base_collections_base.h | 2 +- strings/base_meta.h | 10 ++++-- test/old_tests/UnitTests/TryLookup.cpp | 48 +++++++++++++++++++++++++- 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 9f098b4d8..6e1114a2f 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1877,7 +1877,7 @@ namespace cppwinrt { auto name = method_signature.return_param_name(); - w.write("auto out_param_val = %(%);", + w.write("auto out_param_val = %(%, trylookup_from_abi);", upcall, bind(method_signature)); w.write(R"( @@ -1942,7 +1942,7 @@ namespace cppwinrt format = R"( int32_t __stdcall %(%) noexcept final try { % typename D::abi_guard guard(this->shim()); - if constexpr (has_try_lookup_v) + if constexpr (has_TryLookup_v) { % } diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index 0f94266a1..2b7b5fb44 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -507,7 +507,7 @@ WINRT_EXPORT namespace winrt struct map_view_base : iterable_base, Version> { // specialization of Lookup that avoids throwing the hresult - std::optional TryLookup(K const& key) const + std::optional TryLookup(K const& key, trylookup_from_abi_t) const { [[maybe_unused]] auto guard = static_cast(*this).acquire_shared(); auto pair = static_cast(*this).get_container().find(static_cast(*this).wrap_value(key)); diff --git a/strings/base_meta.h b/strings/base_meta.h index d30f9cefc..580b10171 100644 --- a/strings/base_meta.h +++ b/strings/base_meta.h @@ -10,6 +10,10 @@ WINRT_EXPORT namespace winrt struct take_ownership_from_abi_t {}; inline constexpr take_ownership_from_abi_t take_ownership_from_abi{}; + // Map implementations can implement TryLookup with trylookup_from_abi_t as an optimization + struct trylookup_from_abi_t {}; + inline constexpr trylookup_from_abi_t trylookup_from_abi{}; + template struct com_ptr; @@ -300,9 +304,9 @@ namespace winrt::impl }; template - struct has_try_lookup + struct has_TryLookup { - template ().TryLookup(std::declval()))> static constexpr bool get_value(int) { return true; } + template ().TryLookup(std::declval(), trylookup_from_abi))> static constexpr bool get_value(int) { return true; } template static constexpr bool get_value(...) { return false; } public: @@ -311,5 +315,5 @@ namespace winrt::impl }; template - inline constexpr bool has_try_lookup_v = has_try_lookup::value; + inline constexpr bool has_TryLookup_v = has_TryLookup::value; } diff --git a/test/old_tests/UnitTests/TryLookup.cpp b/test/old_tests/UnitTests/TryLookup.cpp index 3c6c54580..9abaa5625 100644 --- a/test/old_tests/UnitTests/TryLookup.cpp +++ b/test/old_tests/UnitTests/TryLookup.cpp @@ -143,4 +143,50 @@ TEST_CASE("TryLookup TryRemove error") REQUIRE(!map.TryLookup(123)); REQUIRE(!map.TryRemove(123)); -} \ No newline at end of file +} + +TEST_CASE("trylookup_from_abi specialization") +{ + // A map that throws a specific error, used to verify various edge cases. + // and implements tryLookup, to take advantage of an optimization to avoid a throw. + struct map_with_try_lookup : implements> + { + hresult codeToThrow{ S_OK }; + bool shouldThrowOnTryLookup{ false }; + std::optional TryLookup(int, trylookup_from_abi_t) + { + if (shouldThrowOnTryLookup) + { + throw_hresult(codeToThrow); + } + else + { + return { std::nullopt }; + } + } + int Lookup(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + int32_t Size() { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + bool HasKey(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + void Split(IMapView&, IMapView&) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + }; + + auto self = make_self(); + IMapView map = *self; + + // Make sure that we use the TryLookup specialization, and don't throw an unexpected exception. + self->shouldThrowOnTryLookup = false; + REQUIRE(!map.TryLookup(123)); + // make sure regular lookup stll throws bounds + REQUIRE_THROWS_AS(map.Lookup(123), hresult_out_of_bounds); + + // Simulate a non-agile map that is being accessed from the wrong thread. + // "Try" operations should throw rather than erroneously report "not found". + // Because they didn't even try. The operation never got off the ground. + self->shouldThrowOnTryLookup = true; + self->codeToThrow = RPC_E_WRONG_THREAD; + REQUIRE_THROWS_AS(map.TryLookup(123), hresult_wrong_thread); + // regular lookup should throw the same error + REQUIRE_THROWS_AS(map.Lookup(123), hresult_wrong_thread); + + +} From 18a57a8a90ba63a8e7a4ee8622ba54f15d86063a Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Fri, 19 Sep 2025 16:34:16 -0700 Subject: [PATCH 15/18] address avoid_oritinate comment, no need for a different name, resuse existing hresult_cancelled --- strings/base_coroutine_foundation.h | 8 ++------ strings/base_error.h | 8 +------- test/old_tests/UnitTests/TryLookup.cpp | 2 -- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/strings/base_coroutine_foundation.h b/strings/base_coroutine_foundation.h index 9e7ce581a..694baaa05 100644 --- a/strings/base_coroutine_foundation.h +++ b/strings/base_coroutine_foundation.h @@ -490,7 +490,7 @@ namespace winrt::impl m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed); if (cancellable_promise::avoid_cancel_origination_enabled()) { - m_exception = std::make_exception_ptr(non_originating_hresult_canceled()); + m_exception = std::make_exception_ptr(hresult_canceled(hresult_error::avoid_originate)); } else { @@ -628,10 +628,6 @@ namespace winrt::impl { m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed); } - catch (non_originating_hresult_canceled const&) - { - m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed); - } catch (...) { m_status.store(AsyncStatus::Error, std::memory_order_relaxed); @@ -645,7 +641,7 @@ namespace winrt::impl { if (cancellable_promise::avoid_cancel_origination_enabled()) { - throw winrt::non_originating_hresult_canceled(); + throw winrt::hresult_canceled(hresult_error::avoid_originate); } else { diff --git a/strings/base_error.h b/strings/base_error.h index 2e90b66da..1787f417d 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -332,17 +332,11 @@ WINRT_EXPORT namespace winrt struct hresult_canceled : hresult_error { hresult_canceled(winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, sourceInformation) {} + hresult_canceled(hresult_error::avoid_originate_t) noexcept : hresult_error(impl::error_canceled, hresult_error::avoid_originate) {} hresult_canceled(param::hstring const& message, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, message, sourceInformation) {} hresult_canceled(take_ownership_from_abi_t, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, take_ownership_from_abi, sourceInformation) {} }; - struct non_originating_hresult_canceled : hresult_error - { - non_originating_hresult_canceled(winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, hresult_error::avoid_originate, sourceInformation) {} - non_originating_hresult_canceled(param::hstring const& message, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) = delete; - non_originating_hresult_canceled(take_ownership_from_abi_t , winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) = delete; - }; - [[noreturn]] inline WINRT_IMPL_NOINLINE void throw_hresult(hresult const result, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) { if (winrt_throw_hresult_handler) diff --git a/test/old_tests/UnitTests/TryLookup.cpp b/test/old_tests/UnitTests/TryLookup.cpp index 9abaa5625..4772cf0ac 100644 --- a/test/old_tests/UnitTests/TryLookup.cpp +++ b/test/old_tests/UnitTests/TryLookup.cpp @@ -187,6 +187,4 @@ TEST_CASE("trylookup_from_abi specialization") REQUIRE_THROWS_AS(map.TryLookup(123), hresult_wrong_thread); // regular lookup should throw the same error REQUIRE_THROWS_AS(map.Lookup(123), hresult_wrong_thread); - - } From 2ee9368732ea9d84d4036d3eb650dc2f42c0a720 Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Wed, 24 Sep 2025 18:08:31 -0700 Subject: [PATCH 16/18] change out async to a new name, with it being true by default. add new unittest to test nullable lookup. --- .runsettings | 1 - cppwinrt/code_writers.h | 3 +- strings/base_coroutine_foundation.h | 16 ++++---- strings/base_coroutine_threadpool.h | 10 ++--- strings/base_error.h | 8 ++-- test/old_tests/UnitTests/TryLookup.cpp | 55 ++++++++++++++++++++++++++ test/test/async_check_cancel.cpp | 2 +- 7 files changed, 75 insertions(+), 20 deletions(-) diff --git a/.runsettings b/.runsettings index 075c5e97c..5f10e44f8 100644 --- a/.runsettings +++ b/.runsettings @@ -13,7 +13,6 @@ Single (?i:Test) true - 30000 on true Verbose diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 6e1114a2f..69554d869 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1881,7 +1881,7 @@ namespace cppwinrt upcall, bind(method_signature)); w.write(R"( - if (out_param_val) + if (out_param_val.has_value()) { *% = detach_from<%>(std::move(*out_param_val)); } @@ -1934,6 +1934,7 @@ namespace cppwinrt std::string upcall = "this->shim()."; auto name = get_name(method); upcall += name; + // if (type_name == "Windows.Foundation.Collections.IMapView`2") if (name == "Lookup") { // Special-case Lookup to look for a TryLookup here, to avoid a throw/originate diff --git a/strings/base_coroutine_foundation.h b/strings/base_coroutine_foundation.h index 694baaa05..87aaed24e 100644 --- a/strings/base_coroutine_foundation.h +++ b/strings/base_coroutine_foundation.h @@ -351,9 +351,9 @@ namespace winrt::impl return m_promise->enable_cancellation_propagation(value); } - bool avoid_cancel_origination(bool value = true) const noexcept + bool originate_on_cancel(bool value = true) const noexcept { - return m_promise->enable_avoid_cancel_origination(value); + return m_promise->originate_on_cancel(value); } private: @@ -488,13 +488,13 @@ namespace winrt::impl if (m_status.load(std::memory_order_relaxed) == AsyncStatus::Started) { m_status.store(AsyncStatus::Canceled, std::memory_order_relaxed); - if (cancellable_promise::avoid_cancel_origination_enabled()) + if (cancellable_promise::originate_on_cancel()) { - m_exception = std::make_exception_ptr(hresult_canceled(hresult_error::avoid_originate)); + m_exception = std::make_exception_ptr(hresult_canceled()); } else { - m_exception = std::make_exception_ptr(hresult_canceled()); + m_exception = std::make_exception_ptr(hresult_canceled(hresult_error::no_originate)); } cancel = std::move(m_cancel); } @@ -639,13 +639,13 @@ namespace winrt::impl { if (Status() == AsyncStatus::Canceled) { - if (cancellable_promise::avoid_cancel_origination_enabled()) + if (cancellable_promise::originate_on_cancel()) { - throw winrt::hresult_canceled(hresult_error::avoid_originate); + throw winrt::hresult_canceled(); } else { - throw winrt::hresult_canceled(); + throw winrt::hresult_canceled(hresult_error::no_originate); } } diff --git a/strings/base_coroutine_threadpool.h b/strings/base_coroutine_threadpool.h index ba7bfe3a1..7fa8789c7 100644 --- a/strings/base_coroutine_threadpool.h +++ b/strings/base_coroutine_threadpool.h @@ -180,14 +180,14 @@ WINRT_EXPORT namespace winrt return m_propagate_cancellation; } - bool enable_avoid_cancel_origination(bool value) noexcept + bool originate_on_cancel(bool value = true) noexcept { - return std::exchange(m_avoid_cancel_origination, value); + return std::exchange(m_originate_on_cancel, value); } - bool avoid_cancel_origination_enabled() const noexcept + bool should_originate_on_cancel() const noexcept { - return m_avoid_cancel_origination; + return m_originate_on_cancel; } private: @@ -196,7 +196,7 @@ WINRT_EXPORT namespace winrt std::atomic m_canceller{ nullptr }; void* m_context{ nullptr }; bool m_propagate_cancellation{ false }; - bool m_avoid_cancel_origination{ false }; + bool m_originate_on_cancel{ true }; // By default, will call RoOriginateError before throwing a cancel error code. }; template diff --git a/strings/base_error.h b/strings/base_error.h index 1787f417d..e70c06790 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -84,8 +84,8 @@ WINRT_EXPORT namespace winrt { struct hresult_error { - struct avoid_originate_t {}; - static constexpr avoid_originate_t avoid_originate{}; + struct no_originate_t {}; + static constexpr no_originate_t no_originate{}; using from_abi_t = take_ownership_from_abi_t; static constexpr auto from_abi{ take_ownership_from_abi }; @@ -112,7 +112,7 @@ WINRT_EXPORT namespace winrt originate(code, nullptr, sourceInformation); } - explicit hresult_error(hresult const code, avoid_originate_t, [[maybe_unused]] winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : m_code(verify_error(code)) + explicit hresult_error(hresult const code, no_originate_t, [[maybe_unused]] winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : m_code(verify_error(code)) { } @@ -332,7 +332,7 @@ WINRT_EXPORT namespace winrt struct hresult_canceled : hresult_error { hresult_canceled(winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, sourceInformation) {} - hresult_canceled(hresult_error::avoid_originate_t) noexcept : hresult_error(impl::error_canceled, hresult_error::avoid_originate) {} + hresult_canceled(hresult_error::no_originate_t) noexcept : hresult_error(impl::error_canceled, hresult_error::no_originate) {} hresult_canceled(param::hstring const& message, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, message, sourceInformation) {} hresult_canceled(take_ownership_from_abi_t, winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : hresult_error(impl::error_canceled, take_ownership_from_abi, sourceInformation) {} }; diff --git a/test/old_tests/UnitTests/TryLookup.cpp b/test/old_tests/UnitTests/TryLookup.cpp index 4772cf0ac..2f335254e 100644 --- a/test/old_tests/UnitTests/TryLookup.cpp +++ b/test/old_tests/UnitTests/TryLookup.cpp @@ -188,3 +188,58 @@ TEST_CASE("trylookup_from_abi specialization") // regular lookup should throw the same error REQUIRE_THROWS_AS(map.Lookup(123), hresult_wrong_thread); } + +TEST_CASE("trylookup_from_abi specialization with IInspectable") +{ + // A map that throws a specific error, used to verify various edge cases. + // and implements tryLookup, to take advantage of an optimization to avoid a throw. + struct map_with_try_lookup : implements> + { + hresult codeToThrow{ S_OK }; + bool shouldThrowOnTryLookup{ false }; + bool returnNullptr{ false }; + std::optional TryLookup(int, trylookup_from_abi_t) + { + if (returnNullptr) + { + return { nullptr }; + } + else if (shouldThrowOnTryLookup) + { + throw_hresult(codeToThrow); + } + else + { + return { std::nullopt }; + } + } + IInspectable Lookup(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + int32_t Size() { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + bool HasKey(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + void Split(IMapView&, IMapView&) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + }; + + auto self = make_self(); + IMapView map = *self; + + // Ensure that we return a value on nullptr, a nullptr is a valid IInspectable in the Map + self->returnNullptr = true; + REQUIRE(map.TryLookup(123) == IInspectable{nullptr}); + REQUIRE(map.Lookup(123) == IInspectable{nullptr}); + + // Make sure that we use the TryLookup specialization, and don't throw an unexpected exception. + self->shouldThrowOnTryLookup = false; + self->returnNullptr = false; + REQUIRE(map.TryLookup(123) == IInspectable{nullptr}); + // make sure regular lookup stll throws bounds + REQUIRE_THROWS_AS(map.Lookup(123), hresult_out_of_bounds); + + // Simulate a non-agile map that is being accessed from the wrong thread. + // "Try" operations should throw rather than erroneously report "not found". + // Because they didn't even try. The operation never got off the ground. + self->shouldThrowOnTryLookup = true; + self->codeToThrow = RPC_E_WRONG_THREAD; + REQUIRE_THROWS_AS(map.TryLookup(123), hresult_wrong_thread); + // regular lookup should throw the same error + REQUIRE_THROWS_AS(map.Lookup(123), hresult_wrong_thread); +} diff --git a/test/test/async_check_cancel.cpp b/test/test/async_check_cancel.cpp index 11c902c71..d88fdcaf0 100644 --- a/test/test/async_check_cancel.cpp +++ b/test/test/async_check_cancel.cpp @@ -133,7 +133,7 @@ namespace winrt_throw_hresult_handler = exceptionLogger; auto cancel = co_await get_cancellation_token(); - cancel.avoid_cancel_origination(true); + cancel.originate_on_cancel(false); co_await resume_on_signal(event); From 050fa7a994336553b57c63463ff5126a78d37abc Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Thu, 25 Sep 2025 18:28:01 -0700 Subject: [PATCH 17/18] Address hresult comments by adding tests and removing source_locatoin, special-cased further by checking the typename. --- cppwinrt/code_writers.h | 15 ++++---- strings/base_collections_base.h | 4 +- strings/base_error.h | 2 +- strings/base_meta.h | 2 - test/test_cpp20/custom_error.cpp | 63 +++++++++++++++++++++++++++++++- 5 files changed, 72 insertions(+), 14 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 69554d869..35e9935d0 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1903,7 +1903,7 @@ namespace cppwinrt } } - static void write_produce_method(writer& w, MethodDef const& method) + static void write_produce_method(writer& w, MethodDef const& method, TypeDef const& type) { std::string_view format; @@ -1934,11 +1934,12 @@ namespace cppwinrt std::string upcall = "this->shim()."; auto name = get_name(method); upcall += name; - // if (type_name == "Windows.Foundation.Collections.IMapView`2") - if (name == "Lookup") + + auto typeName = type.TypeName(); + if (((typeName == "IMapView`2") || (typeName == "IMap`2")) + && (name == "Lookup")) { - // Special-case Lookup to look for a TryLookup here, to avoid a throw/originate - // and add a small performance improvement. + // Special-case IMap*::Lookup to look for a TryLookup here, to avoid extranous throw/originates std::string tryLookupUpCall = "this->shim().TryLookup"; format = R"( int32_t __stdcall %(%) noexcept final try { @@ -2012,7 +2013,7 @@ namespace cppwinrt break; } - w.write_each(info.type.MethodList()); + w.write_each(info.type.MethodList(), info.type); } } @@ -2034,7 +2035,7 @@ namespace cppwinrt bind(generics), type, type, - bind_each(type.MethodList()), + bind_each(type.MethodList(), type), bind(type)); } diff --git a/strings/base_collections_base.h b/strings/base_collections_base.h index 2b7b5fb44..fec3de660 100644 --- a/strings/base_collections_base.h +++ b/strings/base_collections_base.h @@ -514,10 +514,10 @@ WINRT_EXPORT namespace winrt if (pair == static_cast(*this).get_container().end()) { - return { std::nullopt }; + return std::nullopt; } - return { static_cast(*this).unwrap_value(pair->second) }; + return static_cast(*this).unwrap_value(pair->second); } V Lookup(K const& key) const diff --git a/strings/base_error.h b/strings/base_error.h index e70c06790..41929336b 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -112,7 +112,7 @@ WINRT_EXPORT namespace winrt originate(code, nullptr, sourceInformation); } - explicit hresult_error(hresult const code, no_originate_t, [[maybe_unused]] winrt::impl::slim_source_location const& sourceInformation = winrt::impl::slim_source_location::current()) noexcept : m_code(verify_error(code)) + explicit hresult_error(hresult const code, no_originate_t) noexcept : m_code(verify_error(code)) { } diff --git a/strings/base_meta.h b/strings/base_meta.h index 580b10171..7dbb4c386 100644 --- a/strings/base_meta.h +++ b/strings/base_meta.h @@ -308,9 +308,7 @@ namespace winrt::impl { template ().TryLookup(std::declval(), trylookup_from_abi))> static constexpr bool get_value(int) { return true; } template static constexpr bool get_value(...) { return false; } - public: - static constexpr bool value = get_value(0); }; diff --git a/test/test_cpp20/custom_error.cpp b/test/test_cpp20/custom_error.cpp index d7b055e47..b925628d9 100644 --- a/test/test_cpp20/custom_error.cpp +++ b/test/test_cpp20/custom_error.cpp @@ -37,9 +37,9 @@ namespace #if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION < 170000 // not available in libc++ before LLVM 16 -TEST_CASE("custom_error_logger", "[!shouldfail]") +TEST_CASE("custom_error_logger_on_throw", "[!shouldfail]") #else -TEST_CASE("custom_error_logger") +TEST_CASE("custom_error_logger_on_throw") #endif { // Set up global handler @@ -72,3 +72,62 @@ TEST_CASE("custom_error_logger") winrt_throw_hresult_handler = nullptr; s_loggerCalled = false; } +template +void HresultOnLine80(Args... args) +{ + // Validate that handler translated on creating an HRESULT +#line 80 // Force next line to be reported as line number 80 + winrt::hresult_canceled(std::forward(args)...); +} + +#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION < 170000 +// not available in libc++ before LLVM 16 +TEST_CASE("custom_error_logger_on_originate", "[!shouldfail]") +#else +TEST_CASE("custom_error_logger_on_originate") +#endif +{ + // Set up global handler + REQUIRE(!s_loggerCalled); + REQUIRE(!winrt_throw_hresult_handler); + winrt_throw_hresult_handler = logger; + + HresultOnLine80(); + REQUIRE(s_loggerCalled); + // In C++20 these fields should be filled in by std::source_location + REQUIRE(s_loggerArgs.lineNumber == 80); + const auto fileNameSv = std::string_view(s_loggerArgs.fileName); + REQUIRE(!fileNameSv.empty()); + REQUIRE(fileNameSv.find("custom_error.cpp") != std::string::npos); +#ifdef _DEBUG + const auto functionNameSv = std::string_view(s_loggerArgs.functionName); + REQUIRE(!functionNameSv.empty()); + // Every compiler has a slightly different naming approach for this function, and even the same + // compiler can change its mind over time. Instead of matching the entire function name just + // match against the part we care about. + REQUIRE((functionNameSv.find("HresultOnLine80") != std::string_view::npos)); +#else + REQUIRE(s_loggerArgs.functionName == nullptr); +#endif // _DEBUG + + REQUIRE(s_loggerArgs.returnAddress); + REQUIRE(s_loggerArgs.result == HRESULT_FROM_WIN32(ERROR_CANCELLED)); // E_ILLEGAL_DELEGATE_ASSIGNMENT) + + s_loggerCalled = false; + s_loggerArgs.lineNumber = 0; + // verify HRESULT with a custom message + HresultOnLine80(L"with custom message"); + REQUIRE(s_loggerCalled); + REQUIRE(s_loggerArgs.lineNumber == 80); + + s_loggerCalled = false; + s_loggerArgs.lineNumber = 0; + // verify that no_originate does _not_ call the logger. + HresultOnLine80(winrt::hresult_error::no_originate); + REQUIRE(!s_loggerCalled); + REQUIRE(s_loggerArgs.lineNumber == 0); + + // Remove global handler + winrt_throw_hresult_handler = nullptr; + s_loggerCalled = false; +} From 4632e57ac3c6995f10eea1b4b44b4ccff308bedf Mon Sep 17 00:00:00 2001 From: Antonio Moreno <43587397+antmor@users.noreply.github.com> Date: Tue, 28 Oct 2025 16:34:47 -0700 Subject: [PATCH 18/18] add unittest without specialization, make sure that the behaviour is opt-in --- test/old_tests/UnitTests/TryLookup.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/old_tests/UnitTests/TryLookup.cpp b/test/old_tests/UnitTests/TryLookup.cpp index 2f335254e..350fa9006 100644 --- a/test/old_tests/UnitTests/TryLookup.cpp +++ b/test/old_tests/UnitTests/TryLookup.cpp @@ -189,6 +189,32 @@ TEST_CASE("trylookup_from_abi specialization") REQUIRE_THROWS_AS(map.Lookup(123), hresult_wrong_thread); } +TEST_CASE("trylookup_from_abi NOT opt-in, no special tag") +{ + // Makes sure that an existing TryLookup method is not called without the trylookup_from_abi_t tag. + struct map_without_try_lookup : implements> + { + hresult codeToThrow{ S_OK }; + std::optional TryLookup(int) // notice no trylookup_from_abi_t, so no opt-in + { + // throw an unexpectd hresult, this should not be called. + throw_hresult(RPC_E_WRONG_THREAD); + } + int Lookup(int) { return 42; } // Behave as if the item was found + + int32_t Size() { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + bool HasKey(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + void Split(IMapView&, IMapView&) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test + }; + + auto self = make_self(); + IMapView map = *self; + + // Make sure that we don't use the TryLookup specialization, we use the Successful Lookup + REQUIRE(map.TryLookup(123).value() == 42); + REQUIRE(map.Lookup(123) == 42); +} + TEST_CASE("trylookup_from_abi specialization with IInspectable") { // A map that throws a specific error, used to verify various edge cases.