Skip to content

<system_error>: Fix the default constructor of error_category conditionally#3139

Merged
StephanTLavavej merged 10 commits intomicrosoft:mainfrom
frederick-vs-ja:error_category-ctor
Oct 24, 2022
Merged

<system_error>: Fix the default constructor of error_category conditionally#3139
StephanTLavavej merged 10 commits intomicrosoft:mainfrom
frederick-vs-ja:error_category-ctor

Conversation

@frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Oct 1, 2022

Fixes #3119.

Addresses #1116 (fixes it except for /clr:pure) and unblocks one libcxx test.

This PR would complicate @vinniefalco's #3044 if it gets merged.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner October 1, 2022 22:57
strega-nil-ms
strega-nil-ms approved these changes Oct 3, 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.

I don't love that we have to do this to keep ABI, but this is certainly better than the alternative where this isn't constexpr.

@CaseyCarter CaseyCarter added the bug Something isn't working label Oct 4, 2022
StephanTLavavej
StephanTLavavej reviewed Oct 20, 2022
View reviewed changes
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 20, 2022

Thanks, this is awesome! 😻 I think we can regard #1116 as completely fixed by this (/clr:pure has virtually no users, and there's nothing more we can do here, so no need to keep a bug open; everything mentioning /clr:pure will inherently be eradicated during vNext).

I pushed a conflict-free and no-reformatting-needed merge with main, followed by a couple of minor cleanups for /clr:pure, and a small enhancement adding noexcept to the _Addr_storage ctors so the compiler won't emit EH enforcement in debug mode (as the ctors that use this are marked noexcept). FYI @strega-nil-ms as you previously approved.

@StephanTLavavej StephanTLavavej self-assigned this Oct 21, 2022
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 21, 2022

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

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 22, 2022

I've pushed an additional commit to silence a warning found by internal testing.

@StephanTLavavej StephanTLavavej merged commit 569cf9b into microsoft:main Oct 24, 2022
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 24, 2022

Thanks for fixing this bug we thought was unfixable! 🐞 🛠️ 😻

This was referenced Oct 24, 2022
Closed
Closed
@frederick-vs-ja frederick-vs-ja deleted the error_category-ctor branch October 24, 2022 23:25
@StephanTLavavej StephanTLavavej mentioned this pull request Oct 25, 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

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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.

<system_error>: Can we add constexpr to the default constructor of error_category without ABI breakage?

4 participants

@frederick-vs-ja @StephanTLavavej @strega-nil-ms @CaseyCarter