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.

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

<expected>: Workaround LLVM-59854 #3326

Merged
merged 3 commits into from
Jan 12, 2023
Merged

<expected>: Workaround LLVM-59854 #3326

merged 3 commits into from
Jan 12, 2023

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented Jan 6, 2023 •
edited
Loading

Clang is confused by explicit noexcept on a constexpr destructor for (I think) objects with non-trivially destructible anonymous union members. The workaround is to remove the explicit noexcept and let the compiler default it. I think this is fine as a permanent condition, so I've added no transition comments.

@CaseyCarter
Workaround llvm-project#59854 in expected
e73acca 
Clang is confused by explicit `noexcept` on a constexpr destructor for (I think) objects with non-trivially destructible anonymous union members. The workaround is to remove the explicit `noexcept` and let the compiler default it. I think this is fine as a permanent condition, so I've added no transition comments.
@CaseyCarter CaseyCarter added the bug Something isn't working label Jan 6, 2023
@CaseyCarter CaseyCarter requested a review from a team as a code owner January 6, 2023 06:50
@frederick-vs-ja
Copy link
Contributor

Oh, IIUC this change is violating [res.on.exception.handling]/3? If T or E has a destructor that is noexcept(false), then the destructor with implicit exception specification is also noexcept(false). Although IMO [res.on.exception.handling]/3 is a bit defective, it seemly forbids leaving the destructor of pair, tuple, or array implicitly declared.

@CaseyCarter
Copy link
Member Author

CaseyCarter commented Jan 7, 2023 •
edited
Loading

Oh, IIUC this change is violating [res.on.exception.handling]/3? If T or E has a destructor that is noexcept(false), then the destructor with implicit exception specification is also noexcept(false). Although IMO [res.on.exception.handling]/3 is a bit defective, it seemly forbids leaving the destructor of pair, tuple, or array implicitly declared.

Yes, [res.on.exception.handling]/3 is, err, not perfect. But this is a good enough argument to convince me to restrict the workaround - I'll do so.

@CaseyCarter CaseyCarter changed the title Workaround llvm-project#59854 in expected Workaround llvm/llvm-project#59854 in expected Jan 7, 2023
@CaseyCarter CaseyCarter changed the title Workaround llvm/llvm-project#59854 in expected Workaround LLVM-59854 in expected Jan 9, 2023
CaseyCarter
CaseyCarter commented Jan 9, 2023
View reviewed changes
stl/inc/expected Outdated Show resolved Hide resolved
stl/inc/expected Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter changed the title Workaround LLVM-59854 in expected <expected>: Workaround LLVM-59854 Jan 9, 2023
StephanTLavavej
StephanTLavavej approved these changes Jan 10, 2023
View reviewed changes
stl/inc/expected Show resolved Hide resolved
tests/std/tests/P0323R12_expected/test.cpp Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Jan 11, 2023
@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 7045ec5 into microsoft:main Jan 12, 2023
@StephanTLavavej
Copy link
Member

Thanks for reporting and working around this compiler bug! 🐞 🛠️ ✅

@CaseyCarter CaseyCarter deleted the expected-fix branch January 12, 2023 01:54
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Jan 12, 2023
@CaseyCarter
STL's comments from microsoft#3326
fbcb9ba
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Jan 22, 2023
@CaseyCarter
STL's comments from microsoft#3326
c86e728
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Jan 28, 2023
@CaseyCarter
STL's comments from microsoft#3326
2bb7720
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

Assignees

@StephanTLavavej StephanTLavavej

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CaseyCarter @frederick-vs-ja @StephanTLavavej
  翻译：