Skip to content

Mark several functions as noexcept. #1937

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.

Sign up for GitHub

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

Jump to bottom
Merged
StephanTLavavej merged 16 commits into from
Aug 18, 2022
Merged

Mark several functions as noexcept. #1937

StephanTLavavej merged 16 commits into from
Aug 18, 2022

Conversation

@ThomasFWise
Copy link
Contributor

@ThomasFWise ThomasFWise commented May 27, 2021 •
edited by strega-nil-ms
Loading

This can be important for performance on x86 when calling these functions from code that is marked noexcept.

Drive by: fix optional::operator CMP to correctly only expect an implicit conversion to bool; example fail: https://gcc.godbolt.org/z/xojTqn3vx

@ThomasFWise ThomasFWise requested a review from a team as a code owner May 27, 2021 17:48
@CaseyCarter CaseyCarter added the performance Must go faster label May 27, 2021
@SuperWig
Copy link
Contributor

SuperWig commented May 27, 2021

If I'm not mistaken, if it's not specified as noexcept in the standard then they need to be commented as // strengthened to signify that.

AdamBucior
AdamBucior reviewed May 27, 2021
View reviewed changes
@CaseyCarter CaseyCarter self-assigned this Jun 9, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jul 20, 2021

I've pushed a merge with main to resolve conflicts in <vector> after the toolset update removed _CONSTEXPR20_CONTAINER (it's now _CONSTEXPR20 or plain constexpr), followed by running clang-format.

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Aug 25, 2021

I merged main yet again and resolved a trivial conflict in <optional> where one of the recently removed banner comments appeared in the context around a change.

CaseyCarter
CaseyCarter suggested changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Publishing an ancient partial review.

@ThomasFWise @strega-nil
Mark several functions as noexcept. This is important for performance…
73e8da6 
… on x86 when calling these functions from code that is marked noexcept.
@strega-nil
CRs:
7baadb1 
- AdamBucior: remove noexcept from compare operators on sequence types
- Casey:
   - _Implicitly_convert_to (_Fake_copy_init)
   - /* strengthened */
CaseyCarter
CaseyCarter suggested changes Aug 12, 2022
View reviewed changes
CaseyCarter
CaseyCarter reviewed Aug 12, 2022
View reviewed changes
CaseyCarter
CaseyCarter reviewed Aug 12, 2022
View reviewed changes
CaseyCarter
CaseyCarter suggested changes Aug 12, 2022
View reviewed changes
@strega-nil
fix {} () mismatch in move_iterator
b51c536
@strega-nil
Casey CRs
1d6cdf6
@strega-nil
arrrrgh
5e05e92
@strega-nil
fix tests
9afa779 
Additionally, this fixes a few existing XFAILs in libc++'s test suite
CaseyCarter
CaseyCarter suggested changes Aug 12, 2022
View reviewed changes
CaseyCarter
CaseyCarter approved these changes Aug 13, 2022
View reviewed changes
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 16, 2022

Merge conflicts need to be resolved now that #1347 has landed. 🛬

@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Aug 17, 2022

Merged (and fixed an issue I noticed while merging)

StephanTLavavej
StephanTLavavej reviewed Aug 17, 2022
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll validate and push changes for the small issues I found (only one was significant, regarding a missing noexcept annotation on the tagged base class constructor).

StephanTLavavej
StephanTLavavej reviewed Aug 17, 2022
View reviewed changes
StephanTLavavej
StephanTLavavej reviewed Aug 17, 2022
View reviewed changes
strega-nil-ms
strega-nil-ms suggested changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking request changes as I do not wish to merge without finishing the {_Implicitly_convert_to, _Fake_decay_copy, _Fake_copy_init} discussion.

@strega-nil-ms strega-nil-ms added the decision needed We need to choose something before working on this label Aug 17, 2022
@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label Aug 18, 2022
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 18, 2022

We've decided to go with @strega-nil-ms's proposed unification to _Fake_copy_init in a followup PR (to avoid mixing a large mechanical change with a behavioral change).

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 18, 2022

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

@StephanTLavavej StephanTLavavej merged commit ae0c274 into microsoft:main Aug 18, 2022
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 18, 2022

@ThomasFWise Thanks for strengthening these exception specifications, and congratulations on your first microsoft/STL commit!

Also thanks to @strega-nil-ms for landing this PR!

🎉 😻 🦾

@StephanTLavavej StephanTLavavej mentioned this pull request Aug 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+3 more reviewers

@AdamBucior AdamBucior AdamBucior left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@strega-nil-ms strega-nil-ms strega-nil-ms approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

performance Must go faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ThomasFWise @SuperWig @StephanTLavavej @CaseyCarter @strega-nil-ms @AdamBucior @strega-nil