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.

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

<xiosbase>: Remove out-of-class definitions of static constexpr data members #3381

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

Fixes #3379.

It seems that MSVC and Clang (seemly only in clang-cl mode, though) backport the changes of static constexpr data members in P0386R2 to C++14 mode, which should be a conforming extension IIRC (Godbolt example for MSVC). So out-of-class definitions are seemly needless in MSVC STL, even in C++14 mode.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner February 3, 2023 02:34
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Feb 3, 2023
@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews via automation Feb 3, 2023
@StephanTLavavej
Copy link
Member

I verified that the exports of msvcp140_oss.dll and msvcp140d_oss.dll are unaffected. Both /permissive- and (the infamous, the accursed, the intentionally-no-longer-tested) /Za are fine with this.

Unless we encounter problems in the internal build (e.g. bizarre /clr or /clr:pure interactions), I think this is good. The earlier <random> changes in #3006 were fine, and as I mentioned there, this is how integral_constant::value behaves.

@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Feb 3, 2023
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Feb 3, 2023
@StephanTLavavej StephanTLavavej self-assigned this Feb 10, 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 cce41b2 into microsoft:main Feb 10, 2023
Code Reviews automation moved this from Ready To Merge to Done Feb 10, 2023
@StephanTLavavej
Copy link
Member

Thanks for fixing these compiler warnings and making the codebase less verbose! 🛠️ ⚠️ 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
3 participants