Skip to content

<system_error> Enable [[msvc::noop_dtor]] from Visual Studio 2019 version 16.8. #1016

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
BillyONeal merged 12 commits into from
Jul 21, 2020
Merged

<system_error> Enable [[msvc::noop_dtor]] from Visual Studio 2019 version 16.8. #1016

BillyONeal merged 12 commits into from
Jul 21, 2020

Conversation

@BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Jul 8, 2020

No description provided.

@BillyONeal BillyONeal requested a review from a team as a code owner July 8, 2020 01:18
@BillyONeal BillyONeal self-assigned this Jul 8, 2020
@sylveon
Copy link
Contributor

sylveon commented Jul 8, 2020

Great, now we just need the entirety of declspec also ported to C++11 attributes 😄

CaseyCarter
CaseyCarter suggested changes Jul 8, 2020
View reviewed changes
@BillyONeal @CaseyCarter
Apply Casey's code review suggestions.
c98779f 
Co-authored-by: Casey Carter <cartec69@gmail.com>
BillyONeal
BillyONeal commented Jul 8, 2020
View reviewed changes
@BillyONeal BillyONeal requested a review from CaseyCarter July 8, 2020 02:08
StephanTLavavej
StephanTLavavej requested changes Jul 8, 2020
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.

Can you add a PR description, explaining briefly what [[msvc::noop_dtor]] is, and why we want to use it in the STL?

@StephanTLavavej

This comment has been minimized.

Sign in to view
@BillyONeal BillyONeal added the performance Must go faster label Jul 8, 2020
@MikeGitb
Copy link

MikeGitb commented Jul 9, 2020

Is there public documentation for this attribute somewhere?

@sylveon
Copy link
Contributor

sylveon commented Jul 9, 2020

If I'm the first result in Google, that probably means no

image

@BillyONeal
Copy link
Member Author

BillyONeal commented Jul 9, 2020

Is there public documentation for this attribute somewhere?

No, it's only in nightly builds of the compiler right now; though the rationale for it is effectively this comment block:

STL/stl/inc/system_error

Line 518 in c98779f

// Specifically, we want the error category objects returned from things like std::system_category() to always

StephanTLavavej
StephanTLavavej requested changes Jul 9, 2020
View reviewed changes
StephanTLavavej
StephanTLavavej requested changes Jul 14, 2020
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.

One more round of questions, then I think this will be good to go! 😸

StephanTLavavej
StephanTLavavej approved these changes Jul 16, 2020
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.

I had questions about ctor noexcept and brace style, but I don't think they should block merging. (I would still like a one-line PR description to be edited in though)

@StephanTLavavej

This comment has been minimized.

Sign in to view
@StephanTLavavej

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@BillyONeal BillyONeal merged commit 6c04496 into microsoft:master Jul 21, 2020
@BillyONeal BillyONeal deleted the no_atexit_dtor branch July 21, 2020 07:55
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Jul 28, 2020
@BillyONeal @CaseyCarter @StephanTLavavej
<system_error> Enable [[msvc::noop_dtor]] from Visual Studio 2019 ver…
b3d7039 
…sion 16.8. (microsoft#1016)

Co-authored-by: Casey Carter <cartec69@gmail.com>
Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>
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

@BillyONeal BillyONeal

Labels

performance Must go faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@BillyONeal @sylveon @StephanTLavavej @MikeGitb @CaseyCarter