Fix ABI conformance of IMap::Remove, add TryRemove #655
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`single_threaded_map()`'s IMap::Remove did not throw `hresult_out_of_range` on attempts to remove a nonexistent key. Now it throws. Failure to remove a nonexistent key does not invalidate iterators, because nothing actually changed. This brings the map implementation in line with the implementations in other projections. Note that this is a breaking change. Code that assumed nonexistent objects could be harmlessly removed will encounter exceptions when run against C++/WinRT implementations. This was, however, a pre-existing bug, because implementations from other projections (C#, C++/CX) always threw under those conditions. Added a TryRemove() method for people who wanted the nonthrowing version. Note that fixing the ABI conformance is required in order for TryRemove to work, because TryRemove relies on the call to Remove failing if the key doesn't exist. (JavaScript doesn't project objects as maps, so there is nothing to validate there.) Tightened the behavior of TryLookup and TryRemove so they propagate RPC failures. Because the inability to remove the item could be due to the server being unavailable, and that's not the same as the item not existing in the collection. Previous code treated loss of server the same as "The item doesn't exist", which is not true: The item could exist, we just were unable to contact the server to find out.
kennykerr
reviewed
Jun 8, 2020
kennykerr
reviewed
Jun 8, 2020
kennykerr
reviewed
Jun 8, 2020
| auto found = container.find(static_cast<D const&>(*this).wrap_value(key)); | ||
| if (found == container.end()) | ||
| { | ||
| throw hresult_out_of_bounds(); |
Collaborator
There was a problem hiding this comment.
Is there a reason why it's important that Remove throws when the key is not found, beyond consistency with other implementations?
Member
Author
There was a problem hiding this comment.
It prevents TryRemove from working correctly.
Collaborator
There was a problem hiding this comment.
Yes, but it sounds like a circular argument. TryRemove needs Remove to change. TryRemove is needed because Remove has changed. 😉
Member
Author
There was a problem hiding this comment.
TryRemove is needed because if the map came from somebody other than C++/WinRT, it will throw. For example, if you want to "TryRemove" from CoreApplication.Properties, you cannot use "Remove" because that property set is implemented in WRL, not C++/WinRT, and it throws on key not found.
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
because they can throw on other errors. Added unit test to verify that errors other than "key not found" are propagated.
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
single_threaded_map()'s IMap::Remove did not throwhresult_out_of_rangeon attempts to remove a nonexistent key. Now it throws.Note that this is a breaking change. Code that assumed nonexistent objects could be harmlessly removed will encounter exceptions when run against C++/WinRT implementations when they hadn't before this change. This was, however, a pre-existing bug, because implementations from other projections (C#, C++/CX, WRL) always throw under those conditions.
Added a TryRemove() method for people who want the nonthrowing version.
Note that fixing the ABI conformance is required in order for TryRemove to return the correct result, because TryRemove relies on the call to Remove failing if the key doesn't exist.
(JavaScript doesn't project objects as maps, so there is nothing to validate there.)
Tightened the behavior of TryLookup and TryRemove so they swallow only "key not found" and propagate other errors. The inability to remove the item could be due to the server being unavailable, or the app accessing the map from the wrong thread, and that's not the same as the item not existing in the collection. Previous code treated these errors the same as "The item doesn't exist", which is not true: The item could exist, we simply don't know since the call never got off the ground.
Failure to remove a nonexistent key no longer invalidates iterators, because nothing actually changed. This brings the map implementation in line with the implementations in C# and WRL. (C++/CX invalidates prematurely. JavaScript never invalidates iterators.)