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

Include <cmeow> instead of <meow.h> #1405

Merged
merged 9 commits into from
Nov 6, 2020

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Oct 27, 2020

  • We no longer need

    STL/stl/inc/xatomic.h

    Lines 14 to 16 in 5f736ef

    #if defined(_WIN64) && (_MSC_FULL_VER <= 192829213) // TRANSITION
    #include <intrin.h> // Visual Studio 2019 to define 128-bit CAS in <intrin0.h>
    #endif // defined(_WIN64) && (_MSC_FULL_VER <= 192829213), TRANSITION

    as VS 2019 16.8 Preview 5 contains this change, and its _MSC_FULL_VER exceeds this value. (Also, the MSVC-internal build always uses the latest <intrin0.h>.)
  • In stl/inc, consistently include <cmeow> instead of <meow.h>. Previously, we had approximately 42 includes of <cmeow> and 26 of <meow.h>, so this reduces inconsistency. There should be essentially no throughput cost (especially because in many cases, <cmeow> was already being included through another path). There is also a slight user experience benefit to consistently using <cmeow> - while unrelated to any Standard guarantee, it is less surprising if we always drag in ::meow and std::meow simultaneously.
  • This doesn't change any other product code - e.g. we still use _CSTD to access names.
  • Change most C wrapper headers to be core headers.
    • These are now core: cassert, cctype, cerrno, cfenv, cinttypes, clocale, csetjmp, csignal, cstdarg, cstring, ctime, cuchar, cwctype.
    • We need to change xstoul.cpp and xstoull.cpp because they're now including only core C wrapper headers, yet they're using the DLL export macro. Therefore, they should directly include yvals.h.
  • Test core headers. Resolves Test "core" headers #1411.
  • <tuple> doesn't need to include <new>.
    • It doesn't mention anything that <new> provides.
    • <xutility> provides uses_allocator and allocator_arg.
  • Small cleanups in xstol.cpp, xstoll.cpp, xstoul.cpp.
    • xstol.cpp doesn't need xmath.hpp, which is for floating-point.
    • xstoll.cpp doesn't need xmath.hpp either, but will need yvals.h.
    • xstoul.cpp had a leftover "macros" comment. Also, remove an empty line and move the "valid digits" comment to be more consistent with xstoull.cpp.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Oct 27, 2020
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner October 27, 2020 06:44
@mnatsuhara mnatsuhara self-assigned this Oct 28, 2020
@StephanTLavavej
Copy link
Member Author

I will need to revise this - we've discovered that some <cmeow> headers are non-core when they should be core.

@CaseyCarter
Copy link
Member

I will need to revise this - we've discovered that some <cmeow> headers are non-core when they should be core.

Clarifying the distinction: "core" headers don't require linking to the DLL(s) to function, so they don't include the pragma that directs the linker to link in msvcp (nor any other C++ DLL). We annotate them by adding (core) to the first-line comment in each such header. Core headers can include other core headers, but no non-core headers.

@StephanTLavavej
Copy link
Member Author

We also have a whole wiki page about them: https://github.com/microsoft/STL/wiki/The-Difference-Between-Core-And-Non-Core-Headers
I think I will add test coverage since this is clearly fragile.

These are now core: cassert, cctype, cerrno, cfenv, cinttypes, clocale,
csetjmp, csignal, cstdarg, cstring, ctime, cuchar, cwctype.

We need to change xstoul.cpp and xstoull.cpp because they're now
including only core C wrapper headers, yet they're using the DLL export
macro. Therefore, they should directly include yvals.h.
It doesn't mention anything that <new> provides.

<xutility> provides uses_allocator and allocator_arg.
xstol.cpp doesn't need xmath.hpp, which is for floating-point.

xstoll.cpp doesn't need xmath.hpp either, but will need yvals.h.

xstoul.cpp had a leftover "macros" comment. Also, remove an empty line
and move the "valid digits" comment to be more consistent
with xstoull.cpp.
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with suggestions.

stl/inc/__msvc_all_public_headers.hpp Outdated Show resolved Hide resolved
tests/std/tests/GH_001411_core_headers/env.lst Outdated Show resolved Hide resolved
StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Nov 5, 2020
@StephanTLavavej StephanTLavavej self-assigned this Nov 5, 2020
StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved test Related to test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test "core" headers
3 participants