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

Various cleanups: Simplify ios_base and locale constants #4125

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Oct 23, 2023

Background

Using static constexpr is what allows the major improvement of dropping out-of-line definitions - see #3381 which made that change to ios_base's base class _Iosb.

<xiosbase>

  • The DLL's exports don't mention any of _Dummy_enum, _Dummy_enum_val, _Stdio, _Openmode, _Openmask, _Seekdir, _Seekbeg, _Seekcur, _Seekend, _Openprot.
  • Move retained enums to the bottom of _Iosb with an improved comment.
    • There's an internal MaxMPDCompat tool that notices the enum types and enumerators. With significant effort I think I could update the checked-in CompatibilityData baseline, but for now this is the easiest cleanup.
  • We can drop _Stdio entirely (it's not an enumerator).
  • We can simplify the definitions of the meowfield constants (I verified that this doesn't change them).
  • The _Seekdir enumerators were being used exactly once; we can simply hardcode their values.

<xlocale>

Simplify locale's category constants.

We need to retain _Locbase and comment it as TRANSITION, ABI because it affects sizeof(locale) due to MSVC's deficient EBCO. (On x64, sizeof(locale) is 16, but removing _Locbase would shrink it to 8.)

However, there appears to be nothing stopping us from moving the constants into locale, as depicted in the Standard. I've verified that the DLL's exports are unaffected, and they don't mention _Locbase at all. (They also don't mention the names of these constants, excluding std::collate, std::ctype, and std::messages which are different.)

I don't believe we need _PGLOBAL - it's rarely used in the STL, and never for new code. crtdefs.h defines it to be __declspec(process) for _M_CEE (i.e. /clr and /clr:pure). https://learn.microsoft.com/en-us/cpp/cpp/process?view=msvc-170 explains "When compiling with /clr, global and static variables are per-process by default and do not need to use __declspec(process)." so this doesn't affect /clr at all, which is what matters. (As for /clr:pure, it's vanishingly unlikely to matter - these are constants, so if multiple application domains start getting multiple copies, who cares?)

using category = int; is mandated by the Standard. We previously said int for the constants, but the Standard depicts category, so I'm following that now that the typedef is in scope.

Finally, I'm upgrading const to constexpr - this is not depicted in the Standard, but I believe it's more consistent with the static constexpr variables we use everywhere these days. Note that [constexpr.functions] forbids constexpr-strengthening functions, but it doesn't care about variables.

We need to retain _Locbase and comment it as TRANSITION, ABI because it affects sizeof(locale) due to MSVC's deficient EBCO.
(On x64, sizeof(locale) is 16, but removing _Locbase would shrink it to 8.)

However, there appears to be nothing stopping us from moving the constants into locale, as depicted in the Standard.
I've verified that the DLL's exports are unaffected, and they don't mention _Locbase at all.
(They also don't mention the names of these constants, excluding std::collate, std::ctype, and std::messages which are different.)

I don't believe we need `_PGLOBAL` - it's rarely used in the STL, and never for new code.
crtdefs.h defines it to be `__declspec(process)` for `_M_CEE` (i.e. `/clr` and `/clr:pure`).
https://learn.microsoft.com/en-us/cpp/cpp/process?view=msvc-170 explains
"When compiling with `/clr`, global and static variables are per-process by default and do not need to use `__declspec(process)`."
so this doesn't affect `/clr` at all, which is what matters.
(As for `/clr:pure`, it's vanishingly unlikely to matter - these are *constants*, so if multiple application domains start getting multiple copies, who cares?)

`using category = int;` is mandated by the Standard. We previously said `int` for the constants, but the Standard depicts `category`,
so I'm following that now that the typedef is in scope.

Finally, I'm upgrading `const` to `constexpr` - this is not depicted in the Standard, but I believe it's more consistent with the `static constexpr`
variables we use everywhere these days. Note that \[constexpr.functions\] forbids constexpr-strengthening functions, but it doesn't care about variables.
Using `static constexpr` is what allows the major improvement of dropping the out-of-line definitions - see GH 3381 which made that change to ios_base's base class _Iosb.
Despite the TRANSITION, ABI comments, the DLL's exports don't mention any of `_Dummy_enum`, `_Dummy_enum_val`, `_Stdio`, `_Openmode`, `_Openmask`, `_Seekdir`, `_Seekbeg`, `_Seekcur`, `_Seekend`, `_Openprot`.

`_Dummy_enum` was commented "don't ask" before I got here, and I never knew why. I remember that at some point I tried to mess with it and encountered trouble, which is when the "TRANSITION, ABI" comment was added, but I no longer remember what the problem was. I'm willing to try again, with our vastly increased knowledge of ABI concerns.

The `_Seekdir` enumerators were being used exactly once; we can simply hardcode their values.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Oct 23, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner October 23, 2023 16:52
@github-actions github-actions bot added this to Initial Review in Code Reviews Oct 23, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Oct 23, 2023
stl/inc/xiosbase Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Oct 24, 2023
Co-authored-by: Casey Carter <cacarter@microsoft.com>
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej self-assigned this Oct 24, 2023
@StephanTLavavej
Copy link
Member Author

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

@StephanTLavavej
Copy link
Member Author

I had to push a change to retain the enums in _Iosb, but at least I understand why now.

@StephanTLavavej StephanTLavavej merged commit b7c458a into microsoft:main Oct 26, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Oct 26, 2023
@StephanTLavavej StephanTLavavej deleted the dry-cleaning-9-danger-is-my-middle-name branch October 26, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants