Skip to content

implemented the proposed resolution for LWG-2762#2376

Merged
StephanTLavavej merged 5 commits intomicrosoft:mainfrom
OmarSaber98:LWG_2762
Dec 9, 2021
Merged

implemented the proposed resolution for LWG-2762#2376
StephanTLavavej merged 5 commits intomicrosoft:mainfrom
OmarSaber98:LWG_2762

Conversation

@OmarSaber98
Copy link
Contributor

@OmarSaber98 OmarSaber98 commented Dec 4, 2021

Fixes #2257

@OmarSaber98 OmarSaber98 requested a review from a team as a code owner December 4, 2021 16:47
@ghost
Copy link

ghost commented Dec 4, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@CaseyCarter CaseyCarter added the LWG Library Working Group issue label Dec 4, 2021
CaseyCarter
CaseyCarter suggested changes Dec 4, 2021
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We need a bit of test coverage - basically just a static_assert(noexcept(meow)) where meow is a call to each of these functions. Otherwise this looks great.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Dec 7, 2021

Thanks! I went ahead and pushed comment changes to the test, as it had commented-out lines like // ASSERT_NOT_NOEXCEPT(*opt); (explaining why libcxx couldn't test that, due to their GCC coverage) which are being completely superseded by your STATIC_ASSERTs that these expressions are now required to be noexcept. FYI @CaseyCarter.

@OmarSaber98
Copy link
Contributor Author

OmarSaber98 commented Dec 7, 2021

I tried running the tests on compiler explorer before committing and on the latest msvc the tests were failing with /std:c++latest and /std:c++20 flags but was confused when i found that with /std:c++17 flag the tests were passing.

A reproduction of that can be found here: https://compiler-explorer.com/z/eGbjYof3M

@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Dec 7, 2021 •
edited
Loading

A reproduction of that can be found here: https://compiler-explorer.com/z/eGbjYof3M

/permissive effect. Default of /permissive is different.
Add /permissive- to C++17 to make it fail or add /permissive to C++20 to make it pass.

@StephanTLavavej StephanTLavavej self-assigned this Dec 8, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Dec 8, 2021

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

@StephanTLavavej StephanTLavavej merged commit 524cab2 into microsoft:main Dec 9, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Dec 9, 2021

Thanks for implementing this LWG issue resolution, and congratulations on your second microsoft/STL commit! 😻 ✨ 🚀

This will ship in VS 2022 17.2 Preview 2.

@StephanTLavavej StephanTLavavej mentioned this pull request Dec 9, 2021
Closed
39 tasks
@OmarSaber98 OmarSaber98 deleted the LWG_2762 branch December 9, 2021 04:54
@OmarSaber98 OmarSaber98 restored the LWG_2762 branch December 9, 2021 04:56
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

+1 more reviewer

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

LWG Library Working Group issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LWG-2762 unique_ptr operator*() should be noexcept

4 participants

@OmarSaber98 @StephanTLavavej @AlexGuteniev @CaseyCarter