Skip to content

Commit 783413a

Browse files
committed
added special tag to parameter list to make sure not to break existing TryLookup impls, added test to verify this.
1 parent 7b0072a commit 783413a

File tree

4 files changed

+57
-7
lines changed

4 files changed

+57
-7
lines changed

cppwinrt/code_writers.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1877,7 +1877,7 @@ namespace cppwinrt
18771877
{
18781878
auto name = method_signature.return_param_name();
18791879

1880-
w.write("auto out_param_val = %(%);",
1880+
w.write("auto out_param_val = %(%, trylookup_from_abi);",
18811881
upcall,
18821882
bind<write_produce_args>(method_signature));
18831883
w.write(R"(
@@ -1942,7 +1942,7 @@ namespace cppwinrt
19421942
format = R"( int32_t __stdcall %(%) noexcept final try
19431943
{
19441944
% typename D::abi_guard guard(this->shim());
1945-
if constexpr (has_try_lookup_v<D, K>)
1945+
if constexpr (has_TryLookup_v<D, K>)
19461946
{
19471947
%
19481948
}

strings/base_collections_base.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -507,7 +507,7 @@ WINRT_EXPORT namespace winrt
507507
struct map_view_base : iterable_base<D, Windows::Foundation::Collections::IKeyValuePair<K, V>, Version>
508508
{
509509
// specialization of Lookup that avoids throwing the hresult
510-
std::optional<V> TryLookup(K const& key) const
510+
std::optional<V> TryLookup(K const& key, trylookup_from_abi_t) const
511511
{
512512
[[maybe_unused]] auto guard = static_cast<D const&>(*this).acquire_shared();
513513
auto pair = static_cast<D const&>(*this).get_container().find(static_cast<D const&>(*this).wrap_value(key));

strings/base_meta.h

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ WINRT_EXPORT namespace winrt
1010
struct take_ownership_from_abi_t {};
1111
inline constexpr take_ownership_from_abi_t take_ownership_from_abi{};
1212

13+
// Map implementations can implement TryLookup with trylookup_from_abi_t as an optimization
14+
struct trylookup_from_abi_t {};
15+
inline constexpr trylookup_from_abi_t trylookup_from_abi{};
16+
1317
template <typename T>
1418
struct com_ptr;
1519

@@ -300,9 +304,9 @@ namespace winrt::impl
300304
};
301305

302306
template <typename D, typename K>
303-
struct has_try_lookup
307+
struct has_TryLookup
304308
{
305-
template <typename U, typename = decltype(std::declval<U>().TryLookup(std::declval<K>()))> static constexpr bool get_value(int) { return true; }
309+
template <typename U, typename = decltype(std::declval<U>().TryLookup(std::declval<K>(), trylookup_from_abi))> static constexpr bool get_value(int) { return true; }
306310
template <typename> static constexpr bool get_value(...) { return false; }
307311

308312
public:
@@ -311,5 +315,5 @@ namespace winrt::impl
311315
};
312316

313317
template <typename D, typename K>
314-
inline constexpr bool has_try_lookup_v = has_try_lookup<D, K>::value;
318+
inline constexpr bool has_TryLookup_v = has_TryLookup<D, K>::value;
315319
}

test/old_tests/UnitTests/TryLookup.cpp

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,4 +143,50 @@ TEST_CASE("TryLookup TryRemove error")
143143
REQUIRE(!map.TryLookup(123));
144144
REQUIRE(!map.TryRemove(123));
145145

146-
}
146+
}
147+
148+
TEST_CASE("trylookup_from_abi specialization")
149+
{
150+
// A map that throws a specific error, used to verify various edge cases.
151+
// and implements tryLookup, to take advantage of an optimization to avoid a throw.
152+
struct map_with_try_lookup : implements<map_with_try_lookup, IMapView<int, int>>
153+
{
154+
hresult codeToThrow{ S_OK };
155+
bool shouldThrowOnTryLookup{ false };
156+
std::optional<int> TryLookup(int, trylookup_from_abi_t)
157+
{
158+
if (shouldThrowOnTryLookup)
159+
{
160+
throw_hresult(codeToThrow);
161+
}
162+
else
163+
{
164+
return { std::nullopt };
165+
}
166+
}
167+
int Lookup(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
168+
int32_t Size() { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
169+
bool HasKey(int) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
170+
void Split(IMapView<int, int>&, IMapView<int, int>&) { throw_hresult(E_UNEXPECTED); } // shouldn't be called by the test
171+
};
172+
173+
auto self = make_self<map_with_try_lookup>();
174+
IMapView<int, int> map = *self;
175+
176+
// Make sure that we use the TryLookup specialization, and don't throw an unexpected exception.
177+
self->shouldThrowOnTryLookup = false;
178+
REQUIRE(!map.TryLookup(123));
179+
// make sure regular lookup stll throws bounds
180+
REQUIRE_THROWS_AS(map.Lookup(123), hresult_out_of_bounds);
181+
182+
// Simulate a non-agile map that is being accessed from the wrong thread.
183+
// "Try" operations should throw rather than erroneously report "not found".
184+
// Because they didn't even try. The operation never got off the ground.
185+
self->shouldThrowOnTryLookup = true;
186+
self->codeToThrow = RPC_E_WRONG_THREAD;
187+
REQUIRE_THROWS_AS(map.TryLookup(123), hresult_wrong_thread);
188+
// regular lookup should throw the same error
189+
REQUIRE_THROWS_AS(map.Lookup(123), hresult_wrong_thread);
190+
191+
192+
}

0 commit comments

Comments
 (0)