Hi Jonathan, Sorry for taking a while to look at this.
Jonathan Wakely <jwak...@redhat.com> writes:
> This was approved in Wrocław as LWG 3899.
>
> libstdc++-v3/ChangeLog:
>
> * include/std/generator (generator::yield_value): Add overload
> taking lvalue element_of view, as per LWG 3899.
> ---
>
> The issue suggests that this change avoids creating a new coroutine
> frame, so I thought if I used a custom allocator I'd be able to see a
> reduction in memory allocation after this fix. I was unable to see any
> difference. I would welcome a testcase for it.
>
> Tested x86_64-linux.
>
> libstdc++-v3/include/std/generator | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/libstdc++-v3/include/std/generator
> b/libstdc++-v3/include/std/generator
> index bba85bd0aa4..807e724c0c8 100644
> --- a/libstdc++-v3/include/std/generator
> +++ b/libstdc++-v3/include/std/generator
> @@ -153,6 +153,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> noexcept
> { return _Recursive_awaiter { std::move(__r.range) }; }
>
> + // _GLIBCXX_RESOLVE_LIB_DEFECTS
> + // 3899. co_yielding elements of an lvalue generator is
> + // unnecessarily inefficient
> + template<typename _R2, typename _V2, typename _A2, typename _U2>
> + requires std::same_as<_Yield2_t<_R2, _V2>, _Yielded>
> + auto
> + yield_value(ranges::elements_of<generator<_R2, _V2, _A2>&, _U2> __r)
> + noexcept
> + { return _Recursive_awaiter { std::move(__r.range) }; }
> +
> template<ranges::input_range _R, typename _Alloc>
> requires convertible_to<ranges::range_reference_t<_R>, _Yielded>
> auto
This LGTM. The general range overload of elements_of does make an extra
allocation, the reason you missed it is probably because you didn't pass
the allocator through elements_of (which has dual purpose, passing an
allocator and signifying the range to yield from). This test detects
it:
~/.../testsuite/24_iterators/range_generators$ g++ -I ../../util/ -std=c++23
alloc.cc ../../util/testsuite_allocator.cc && ./a.out
1 320
2 320
1 416
2 416
416
~/.../testsuite/24_iterators/range_generators$ g++ -I . -I ../../util/
-std=c++23 alloc.cc ../../util/testsuite_allocator.cc && ./a.out
1 320
2 320
1 320
2 320
320
~/.../testsuite/24_iterators/range_generators$ cat alloc.cc
#include <generator>
#include <testsuite_hooks.h>
#include <testsuite_allocator.h>
template<typename... Args>
std::generator<int, void, __gnu_test::tracker_allocator<char>>
gen(Args...)
{
co_yield 1;
co_yield 2;
}
template<typename... Args>
std::generator<int, void, __gnu_test::tracker_allocator<char>>
gen2(Args...)
{
// example from the LWG issue 3899
auto g = gen(std::allocator_arg, __gnu_test::tracker_allocator<char>{});
auto g2 = gen(std::allocator_arg, __gnu_test::tracker_allocator<char>{});
co_yield std::ranges::elements_of(std::move(g),
// allocator arg
__gnu_test::tracker_allocator<char>{});
co_yield std::ranges::elements_of(g2,
// allocator arg
__gnu_test::tracker_allocator<char>{});
}
int
main()
{
using __gnu_test::tracker_allocator_counter;
for (auto i : gen2(std::allocator_arg,
__gnu_test::tracker_allocator<char>{}))
__builtin_printf("%d %zu\n", i,
tracker_allocator_counter::get_allocation_count());
__builtin_printf("%zu\n",
tracker_allocator_counter::get_allocation_count());
}
Of course, the actual values are dependent on the coroutine
implementation, so I wouldn't rely on them, but instead on raw
allocation count (but AFAICT the tracker_allocator doesn't count
invocations of allocate(), rather only sizes, so I'd need to write a new
allocator).
In the text above, -I . adds a <generator> with your patch applied
into the include path.
In the future, the extra allocation of the general range might be
removed, but it's probably OK to rely on it for the test for now.
Thanks for the patch. Have a lovely day.
--
Arsen Arsenović
signature.asc
Description: PGP signature
