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

Casey's miscellaneous changes, definitive edition #5014

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

CaseyCarter
Copy link
Member

Nice bite-sized commits:

  • Improve formatting of _Default_allocate_traits; define a new macro instead of conditionally inserting _CONSTEXPR20. Drive-by: Combine nested conditionals.

  • Fix TRANSITION comment for disabled EDG config in P1502R1_standard_library_header_units.

  • Annotate workaround for Clang constrained-friends issue with TRANSITION, Clang 19. I discovered this has been fixed in 19.1.0 after reducing a repro to file, didn't try to dig up the fix.

  • Remove extraneous constexpr in P2502R2_generator

  • Simplify unreachable_sentinel_t. We had to workaround the fact that hidden friends aren't hidden in permissive mode by stuffing unreachable_sentinel_t's operator== into a detail namespace. Now that the changes to C++20 relational expressions are implemented, we can use a member operator== instead of a friend to more simply achieve the same effect. See https://godbolt.org/z/x5ffjdxYf for a proof-of-concept.

  • Uniformly annotate undefined members of fake iterators

  • Cleanup some iterator definitions

    • Remove nested type names that are extraneous in C++20 to help us avoid depending on the existence of said nested type names.
    • Always declare the "nested 5 iterator traits" (or the subset of them a given iterator provides) in canonical order at the top of the first public section in a class body.
    • When the "nested 5 iterator traits" are available, use their names in the semantically appropriate places in member function declarations. For example, reference operator*() const.
  • Rename _Merge_move to _Merge_move_unchecked for consistency with other algorithms that take unwrapped iterators.

  • Improve detection of library-defined array specializations. _Unchecked_begin/_Unchecked_end are semi-documented, so their presence doesn't necessarily indicate a specialization is library-defined. Use our _Is_from_primary tech, and remove the now-unused _Has_unchecked_begin_end.

  • Add some internal checks to internal algorithms. _Uninitialized_copy, _Uninitialized_copy_n, and _Uninitialized_move in <xmemory> are internal-only, and trust that the caller has passed arguments that denote a valid range. I am not so trusting. _STL_INTERNAL_CHECK((_STD _Adl_verify_range(_First, _Last), true)) feels like a bit of a hack - I could be convinced to simply wrap the _Adl_verify_range call in #ifdef _ENABLE_STL_INTERNAL_CHECK.

  • Verify that arguments to subrange's iter/sentinel constructors denote a range. They must per N4988 [range.subrange.ctor] para 1 and 3. As a rule, we never verify that [begin(meow), end(meow)) for a range meow actually denotes a range, so subrange's constructors are the only chance we have to verify these user-supplied values.

CaseyCarter and others added 10 commits October 13, 2024 10:33
Define a new macro instead of conditionally inserting `_CONSTEXPR20`.

Drive-by: Combine nested conditionals.
Discovered this has been fixed in 19.1.0 after reducing a repro to file.
We had to workaround the fact that hidden friends aren't hidden in permissive mode by stuffing `unreachable_sentinel_t`'s `operator==` into a detail namespace. Now that the changes to C++20 relational expressions are implemented, we can use a member `operator==` instead of a `friend` to more simply achieve the same effect. See https://godbolt.org/z/x5ffjdxYf for a proof-of-concept.
* Remove nested type names that are extraneous in C++20 to help us avoid depending on the existence of said nested type names.
* Always declare the "nested 5 iterator traits" (or the subset of them a given iterator provides) in canonical order at the top of the first public section in a class body.
* When the "nested 5 iterator traits" _are_ available, use their names in the semantically appropriate places in member function declarations. For example, `reference operator*() const`.
For consistency with other algorithms that take unwrapped iterators.
`_Unchecked_begin`/`_Unchecked_end` are semi-documented, so their presence doesn't necessarily indicate a specialization is library-defined. Use our `_Is_from_primary` tech, and remove the now-unused `_Has_unchecked_begin_end`.
`_Uninitialized_copy`, `_Uninitialized_copy_n`, and `_Uninitialized_move` in `<xmemory>` are internal-only, and trust that the caller has passed arguments that denote a valid range. I am not so trusting.

`_STL_INTERNAL_CHECK((_STD _Adl_verify_range(_First, _Last), true))` feels like a bit of a hack - I could be convinced to simply wrap the `_Adl_verify_range` call in `#ifdef _ENABLE_STL_INTERNAL_CHECK`.
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Oct 13, 2024
@CaseyCarter CaseyCarter requested a review from a team as a code owner October 13, 2024 18:06
@CaseyCarter
Copy link
Member Author

"I don't need to redo the full test run; surely this small subrange change won't break anything." 🙄

…te a range

They must per N4988 [range.subrange.ctor] para 1 and 3. As a rule, we never verify that `[begin(meow), end(meow))` for a range `meow` actually denotes a range, so `subrange`'s constructors are the only chance we have to verify these user-supplied values.
stl/inc/xutility Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

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

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
Status: Ready To Merge
Development

Successfully merging this pull request may close these issues.

2 participants