Skip to content

<expected>: Workaround LLVM-59854#3326

Merged
StephanTLavavej merged 3 commits into
microsoft:mainfrom
CaseyCarter:expected-fix
Jan 12, 2023
Merged

<expected>: Workaround LLVM-59854#3326
StephanTLavavej merged 3 commits into
microsoft:mainfrom
CaseyCarter:expected-fix

Conversation

@CaseyCarter
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

frederick-vs-ja commented Jan 6, 2023

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
Copy Markdown
Contributor 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
Comment thread stl/inc/expected Outdated
Comment thread stl/inc/expected Outdated
@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
Comment thread stl/inc/expected
Comment thread tests/std/tests/P0323R12_expected/test.cpp
@StephanTLavavej StephanTLavavej self-assigned this Jan 11, 2023
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Jan 11, 2023

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
Copy Markdown
Member

StephanTLavavej commented Jan 12, 2023

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