Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<xtree>: Use scope guard for node copy failure #4749

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

frederick-vs-ja
Copy link
Contributor

Towards #2307.

Also drops an if-statement and reuses _Newroot since _Scary->_Myhead->_Isnil is always true. Closes #1913.

Also removes an `if`-statement since `_Scary->_Myhead->_Isnil` is
always `true`.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner June 26, 2024 18:02
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jun 26, 2024
@StephanTLavavej StephanTLavavej self-assigned this Jun 26, 2024
@StephanTLavavej StephanTLavavej removed their assignment Jun 26, 2024
@AlexGuteniev
Copy link
Contributor

Is there test coverage?

@frederick-vs-ja
Copy link
Contributor Author

frederick-vs-ja commented Jun 27, 2024

Is there test coverage?

I think the partial test coverage is (roughly) in libcxx test's std/containers/associative/from_range_associative_containers.h. The coverage doesn't seem complete though.

Perhaps we should add a test file to MSVC STL which covers copy/move ctors, corresponding allocator-extended ctors, and copy/move assignment operators of associative containers.

@AlexGuteniev
Copy link
Contributor

I expect that non-throwing path is implicitly covered somehow, I think it is enough if there's a test for throwing path

@AlexGuteniev
Copy link
Contributor

I mean I'm asking for the coverage for the guard, not for the unneeded conditional removal.

@StephanTLavavej
Copy link
Member

We didn't have test coverage for the _RERAISE before and the transformation is fairly straightforward so I'm willing to accept this PR as-is, but adding test coverage in a followup that exercises the guard would certainly be reasonable.

@AlexGuteniev
Copy link
Contributor

#2308 has an example of coverage

@StephanTLavavej StephanTLavavej self-assigned this Jul 4, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 153b940 into microsoft:main Jul 5, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for eliminating yet another occurrence of the try-catch-rethrow pattern! 🐈 🐈‍⬛ 😸

@frederick-vs-ja frederick-vs-ja deleted the tree-scope-guard branch July 5, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

<xtree> : Is this determine statement redundant?
3 participants