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

Polish std::generator #4952

Merged
merged 15 commits into from
Sep 13, 2024
Merged

Conversation

CaseyCarter
Copy link
Contributor

This should complete the generator feature branch and get it ready to merge. There are very few product code changes here, it's mostly expanded test coverage with some cleanup and reorganization. There's also some documentation of the "stack of coroutines" and the memory layout of allocations that will make it easier for folks to get up to speed on how the code works under the covers.

It's broken down nicely by commit for ease of review; I'll replicate the commit messages here as an overview. Comments without specific context refer to P2502R2_generator where most changes were made. The P2502R2_generator_iterator and P2502R2_generator_promise tests were already very nicely complete.

  • Update generator template argument mandates
    • Update citations to WG21-N4988
    • Reorder checks to specification order
    • Make _RRef more obviously reflect the wording for RRef in WG21-N4988 [coro.generator.class]/1.4.
  • Allocator testing updates
    • Simplify and correct StatelessAlloc
      • Deriving publicly from std::allocator not a great idea, as witnessed by the recent addition of allocate_at_least.
      • deallocate should be noexcept for a Cpp17Allocator.
      • We don't need to reimplement std::allocator when we can simply use it.
      • The domain of equality for Cpp17Allocators is an entire rebind family, i.e., StatelessAlloc<T> and allocator_traits<StatelessAlloc<T>>::rebind_alloc<U> must be comparable.
    • PascalCase stateful_alloc for consistency and move into the header with StatelessAlloc.
  • Promise test tweaks
    • "whose member await_ready returns false" in WG21-N4988 [coro.generator.promise]/11 implies the return type is exactly bool, not "convertible to bool". Update test_yield_elements_of_range accordingly.
    • Move non-portable size check from P2502R2_generator into P2502R2_generator_promise
  • Expand and complete static_checks: Implement a complete generator traits in the header to use in both P2502R2_generator_iterator and P2502R2_generator's static_checks.
  • Create generic test_one template which takes a generator, a description of its static properties, and the expected result of piping the generator through a provided range adaptor. test_one validates the static properties with static_checks, and confirms the output is as expected.
  • Extract test cases for weird reference types (mutable lvalue and rvalue references) from main into new function test_weird_reference_types.
  • Several small tweaks:
    • It's no longer significant that co_upto wasn't an example in the proposal; strike the comment.
    • Enforce co_upto's precondition so it's nicely documented.
    • Consistently prefer same_as to is_same_v.
    • Regroup calls to test cases in main topically and title each category.
  • Reorganize test code to agree with call order in main. [This is the largest individual commit; it is a pure reordering.]
  • All product code assertions now depend on _CONTAINER_DEBUG_LEVEL. These are all simple O(1) checks, there's no reason not to promote them from _DEBUG to _CONTAINER_DEBUG_LEVEL.
  • Clarify allocation mechanisms, including some fancy memory layout diagrams.
    • Rename _Promise_allocator to _Coro_promise_allocator to avoid any confusion with _Promise. Rename its template parameter _Allocator to _Proto_allocator to avoid confusion with the rebound allocator type _Alloc.
    • Expand static_assert message in operator new. If and when _Coro_promise_allocator is reused by other coroutine types we can worry about making the message more generic.
    • Extract block size computations.
  • Element_awaiter must direct-non-list-initialize its stored object, per WG21-N4988 [coro.generator.promise]/7.
  • Document the "stack of coroutine handles" in a code comment with another work of art.

* Update citations to N4988
* Reorder checks to specification order
* Make `_RRef` more obviously reflect the wording
Simplify and correct `StatelessAlloc`
* Deriving publicly from `std::allocator` not a great idea, as witnessed by the recent addition of `allocate_at_least`.
* `deallocate` should be `noexcept` for a `Cpp17Allocator`.
* We don't need to reimplement `std::allocator` when we can simply use it.
* The domain of equality for `Cpp17Allocators` is an entire `rebind` family, i.e., `StatelessAlloc<T>` and `allocator_traits<StatelessAlloc<T>>::rebind_alloc<U>` must be comparable.

PascalCase `stateful_alloc` for consistency and move into the header with `StatelessAlloc`.
"whose member `await_ready` returns `false`" in N4988 [coro.generator.promise]/11 implies the return type is exactly `bool`, not "convertible to `bool`". Update `test_yield_elements_of_range` accordingly.

Move size check from `P2502R2_generator` into `P2502R2_generator_promise`
Implement a complete `generator` traits in the header to use in both `P2502R2_generator_iterator` and `P2502R2_generator`'s `static_checks`.
... which takes a generator, a description of its static properties, and the expected result of piping the generator through a provided range adaptor. `test_one` validates the static properties with `static_checks`, and confirms the output is as expected.
... from `main` into new function `test_weird_reference_types`.
* Verify the default template arguments and the alias mapping
* Ensure the generator doesn't allocate with `new`
* It's no longer significant that `co_upto` wasn't an example in the proposal; strike the comment.
* Enforce `co_upto`'s precondition so it's nicely documented.
* Consistently prefer `same_as` to `is_same_v`.
* Regroup calls to test cases in `main` topically and title each category.
These are all simple O(1) checks, there's no reason not to promote them from `_DEBUG` to `_CONTAINER_DEBUG_LEVEL`.
Including some fancy memory layout diagrams.

* Rename `_Promise_allocator` to `_Coro_promise_allocator` to avoid any confusion with `_Promise`. Rename its template parameter `_Allocator` to `_Proto_allocator` to avoid confusion with the rebound allocator type `_Alloc`.
* Expand `static_assert` message in `operator new`. If and when `_Coro_promise_allocator` is reused by other coroutine types we can worry about making the message more generic.
* Extract block size computations.
Per N4988 [coro.generator.promise]/7.
@CaseyCarter CaseyCarter added the generator C++23 generator label Sep 12, 2024
@CaseyCarter CaseyCarter requested a review from a team as a code owner September 12, 2024 18:50
* Remove unused `polymorphic_allocator<void>` conversion operator from `StatelessAlloc`.
* Make `StatefulAlloc::domain` private.
* Remove unneeded `constexpr` from `StatefulAlloc`.

Drive-by: Correct typo in `<generator>` comment.
@StephanTLavavej StephanTLavavej self-assigned this Sep 12, 2024
@CaseyCarter CaseyCarter merged commit 89283a6 into microsoft:feature/generator Sep 13, 2024
39 checks passed
@CaseyCarter CaseyCarter deleted the generator branch September 13, 2024 02:38
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator C++23 generator
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants