Skip to content

Conversation

@frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Mar 10, 2024

Towards #2936.

Per the synopsis in [coro.generator.class], the iterator and promise_type types are non-template classes, so they should be ADL-proof even if the enclosing generator specialization is ADL-incompatible. _Nested_awaitable is also changed to a nested non-template class to make generator<holder<incomplete>*> yieldable.

_Promise_allocator is currently unchanged because Cpp17Allocator essentially requires allocator types to be ADL-compatible. Perhaps we should change it to a member marked with [[msvc::no_unique_address]] later. This is wrong.

Drive-by change: make the default template arguments of elements_of and its deduction guide conditionally present, which is consistent with polymorphic_allocator.

#if _HAS_CXX20 && defined(__cpp_lib_byte)
_EXPORT_STD template <class _Ty = byte>
#else // ^^^ _HAS_CXX20 && defined(__cpp_lib_byte) / !_HAS_CXX20 || !defined(__cpp_lib_byte) vvv
_EXPORT_STD template <class _Ty>
#endif // ^^^ !_HAS_CXX20 || !defined(__cpp_lib_byte) ^^^
class polymorphic_allocator {

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner March 10, 2024 07:40
@StephanTLavavej StephanTLavavej added bug Something isn't working generator C++23 generator labels Mar 10, 2024
@StephanTLavavej StephanTLavavej self-assigned this Mar 10, 2024
_NODISCARD auto yield_value(_RANGES elements_of<generator<_Rty, _Vty, _Alloc>&&, _Unused> _Elem) noexcept {
return _Nested_awaitable<_Rty, _Vty, _Alloc>{std::move(_Elem.range)};
using _Nested_awaitable = _Nested_awaitable_provider<_Rty, _Vty, _Alloc>::_Awaitable;
return _Nested_awaitable{std::move(_Elem.range)};
Copy link
Member

Choose a reason for hiding this comment

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

We should use _STD in product code.

using validator = holder<incomplete>*;
auto yield_range = []() -> std::generator<validator> {
co_yield ranges::elements_of(
ranges::views::repeat(nullptr, 42) | ranges::views::transform([](std::nullptr_t) { return validator{}; }));
Copy link
Member

Choose a reason for hiding this comment

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

Should include <cstddef> for std::nullptr_t.

@StephanTLavavej
Copy link
Member

Thanks! I'll go ahead and merge this, and then I'll prepare a followup PR to fix the nitpicks I noticed plus a couple other pre-existing issues.

@StephanTLavavej StephanTLavavej merged commit d5c525d into microsoft:feature/generator Mar 12, 2024
@frederick-vs-ja frederick-vs-ja deleted the adl-proof-generator branch March 12, 2024 12:38
@frederick-vs-ja
Copy link
Contributor Author

frederick-vs-ja commented Mar 15, 2024

I previously wrote

_Promise_allocator is currently unchanged because Cpp17Allocator essentially requires allocator types to be ADL-compatible. Perhaps we should change it to a member marked with [[msvc::no_unique_address]] later.

But this paragraph is wrong - _Promise_allocator should be a base class in order to provide static member functions~, and ADL on it (and its template argument) can find undesired overloads~.

I'll make further fix in a new PR. Nope, this is also wong. Perhaps it should be OK to make _Gen_promise_base a class template.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working generator C++23 generator

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants