On Wed, 11 Dec 2024 at 12:17, Arsen Arsenović <ar...@aarsen.me> wrote:
>
> Jonathan Wakely <jwak...@redhat.com> writes:
>
> > This was approved in Wrocław as LWG 3900, so that passing an incorrect
> > argument intended as an allocator will be ill-formed, instead of
> > silently ignored.
> >
> > This also renames the template parameters and function parameters for
> > the allocators, to align with the names in the standard. I found it too
> > confusing to have a parameter _Alloc which doesn't correspond to Alloc
> > in the standard. Rename _Alloc to _Allocator (which the standard calls
> > Allocator) and rename _Na to _Alloc (which the standard calls Alloc).
> >
> > libstdc++-v3/ChangeLog:
> >
> > * include/std/generator (_Promise_alloc): Rename template
> > parameter. Use __alloc_rebind to rebind allocator.
> > (_Promise_alloc::operator new): Replace constraints with a
> > static_assert in the body. Rename allocator parameter.
> > (_Promise_alloc<void>::_M_allocate): Rename allocator parameter.
> > Use __alloc_rebind to rebind allocator.
> > (_Promise_alloc<void>::operator new): Rename allocator
> > parameter.
> > * testsuite/24_iterators/range_generators/alloc.cc: New test.
> > * testsuite/24_iterators/range_generators/lwg3900.cc: New test.
> > ---
> >
> > Tested x86_64-linux.
> >
> > libstdc++-v3/include/std/generator | 55 +++++++++----------
> > .../24_iterators/range_generators/alloc.cc | 45 +++++++++++++++
> > .../24_iterators/range_generators/lwg3900.cc | 16 ++++++
> > 3 files changed, 87 insertions(+), 29 deletions(-)
> > create mode 100644
> > libstdc++-v3/testsuite/24_iterators/range_generators/alloc.cc
> > create mode 100644
> > libstdc++-v3/testsuite/24_iterators/range_generators/lwg3900.cc
>
> LGTM, with one suggestion below.
>
> > diff --git a/libstdc++-v3/include/std/generator
> > b/libstdc++-v3/include/std/generator
> > index 0a14e70064e..16b953e90af 100644
> > --- a/libstdc++-v3/include/std/generator
> > +++ b/libstdc++-v3/include/std/generator
> > @@ -416,13 +416,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > concept _Stateless_alloc =
> > (allocator_traits<_All>::is_always_equal::value
> > && default_initializable<_All>);
> >
> > - template<typename _Alloc>
> > + template<typename _Allocator>
> > class _Promise_alloc
> > {
> > - using _ATr = allocator_traits<_Alloc>;
> > - using _Rebound = typename _ATr::template rebind_alloc<_Alloc_block>;
> > - using _Rebound_ATr = typename _ATr
> > - ::template rebind_traits<_Alloc_block>;
> > + using _Rebound = __alloc_rebind<_Allocator, _Alloc_block>;
> > + using _Rebound_ATr = allocator_traits<_Rebound>;
> > static_assert(is_pointer_v<typename _Rebound_ATr::pointer>,
> > "Must use allocators for true pointers with
> > generators");
> >
> > @@ -465,30 +463,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > public:
> > void*
> > operator new(std::size_t __sz)
> > - requires default_initializable<_Rebound> // _Alloc is non-void
> > + requires default_initializable<_Rebound> // _Allocator is non-void
> > { return _M_allocate({}, __sz); }
> >
> > - template<typename _Na, typename... _Args>
> > + // _GLIBCXX_RESOLVE_LIB_DEFECTS
> > + // 3900. The allocator_arg_t overloads of promise_type::operator new
> > + // should not be constrained
> > + template<typename _Alloc, typename... _Args>
> > void*
> > operator new(std::size_t __sz,
> > - allocator_arg_t, const _Na& __na,
> > + allocator_arg_t, const _Alloc& __a,
> > const _Args&...)
> > - requires convertible_to<const _Na&, _Alloc>
> > {
> > - return _M_allocate(static_cast<_Rebound>(static_cast<_Alloc>(__na)),
> > - __sz);
> > + static_assert(convertible_to<const _Alloc&, _Allocator>);
> > + return _M_allocate(_Rebound(_Allocator(__a)), __sz);
> > }
> >
> > - template<typename _This, typename _Na, typename... _Args>
> > + template<typename _This, typename _Alloc, typename... _Args>
> > void*
> > operator new(std::size_t __sz,
> > const _This&,
> > - allocator_arg_t, const _Na& __na,
> > + allocator_arg_t, const _Alloc& __a,
> > const _Args&...)
> > - requires convertible_to<const _Na&, _Alloc>
> > {
> > - return _M_allocate(static_cast<_Rebound>(static_cast<_Alloc>(__na)),
> > - __sz);
> > + static_assert(convertible_to<const _Alloc&, _Allocator>);
>
> It might be worth giving a suggestion here?
Like this?
/home/jwakely/gcc/15/include/c++/15.0.0/generator:478:25: error:
static assertion failed: the argument that follows std::allocator_arg
must be convertible to the generator's allocator type
478 | static_assert(convertible_to<const _Alloc&, _Allocator>,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Can we be more precise than "the argument"? Is "the coroutine
argument" accurate? The function that the argument is passed to is a
coroutine, so I think that's correct.
>
> > + return _M_allocate(_Rebound(_Allocator(__a)), __sz);
> > }
> >
> > void
> > @@ -578,20 +577,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > }
> > }
> >
> > - template<typename _Na>
> > + template<typename _Alloc>
> > static void*
> > - _M_allocate(const _Na& __na, std::size_t __csz)
> > + _M_allocate(const _Alloc& __a, std::size_t __csz)
> > {
> > - using _Rebound = typename std::allocator_traits<_Na>
> > - ::template rebind_alloc<_Alloc_block>;
> > - using _Rebound_ATr = typename std::allocator_traits<_Na>
> > - ::template rebind_traits<_Alloc_block>;
> > + using _Rebound = __alloc_rebind<_Alloc, _Alloc_block>;
> > + using _Rebound_ATr = allocator_traits<_Rebound>;
> >
> > static_assert(is_pointer_v<typename _Rebound_ATr::pointer>,
> > "Must use allocators for true pointers with
> > generators");
> >
> > _Dealloc_fn __d = &_M_deallocator<_Rebound>;
> > - auto __b = static_cast<_Rebound>(__na);
> > + auto __b = static_cast<_Rebound>(__a);
> > auto __asz = _M_alloc_size<_Rebound>(__csz);
> > auto __nblk = _Alloc_block::_M_cnt(__asz);
> > void* __p = __b.allocate(__nblk);
> > @@ -619,20 +616,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > return __p;
> > }
> >
> > - template<typename _Na, typename... _Args>
> > + template<typename _Alloc, typename... _Args>
> > void*
> > operator new(std::size_t __sz,
> > - allocator_arg_t, const _Na& __na,
> > + allocator_arg_t, const _Alloc& __a,
> > const _Args&...)
> > - { return _M_allocate(__na, __sz); }
> > + { return _M_allocate(__a, __sz); }
> >
> > - template<typename _This, typename _Na, typename... _Args>
> > + template<typename _This, typename _Alloc, typename... _Args>
> > void*
> > operator new(std::size_t __sz,
> > const _This&,
> > - allocator_arg_t, const _Na& __na,
> > + allocator_arg_t, const _Alloc& __a,
> > const _Args&...)
> > - { return _M_allocate(__na, __sz); }
> > + { return _M_allocate(__a, __sz); }
> >
> > void
> > operator delete(void* __ptr, std::size_t __sz) noexcept
> > diff --git a/libstdc++-v3/testsuite/24_iterators/range_generators/alloc.cc
> > b/libstdc++-v3/testsuite/24_iterators/range_generators/alloc.cc
> > new file mode 100644
> > index 00000000000..8671da748dd
> > --- /dev/null
> > +++ b/libstdc++-v3/testsuite/24_iterators/range_generators/alloc.cc
> > @@ -0,0 +1,45 @@
> > +// { dg-do run { target c++23 } }
> > +
> > +#include <generator>
> > +#include <testsuite_hooks.h>
> > +#include <testsuite_allocator.h>
> > +
> > +template<typename... Args>
> > +std::pmr::generator<int>
> > +gen(Args...)
> > +{
> > + co_yield 1;
> > + co_yield 2;
> > +}
> > +
> > +int
> > +main()
> > +{
> > + __gnu_test::memory_resource mr;
> > + for (auto _ : gen())
> > + VERIFY(mr.number_of_active_allocations() == 0);
> > +
> > + for (auto _ : gen(std::allocator_arg))
> > + VERIFY(mr.number_of_active_allocations() == 0);
> > +
> > + for (auto _ : gen(std::allocator_arg, std::pmr::new_delete_resource()))
> > + VERIFY(mr.number_of_active_allocations() == 0);
> > +
> > +#if __cpp_exceptions
> > + try {
> > + for (auto _ : gen(std::allocator_arg,
> > std::pmr::null_memory_resource()))
> > + VERIFY(false);
> > + } catch (const std::bad_alloc&) {
> > + }
> > +#endif
> > +
> > + VERIFY(mr.number_of_active_allocations() == 0);
> > +
> > + for (auto _ : gen(std::allocator_arg , &mr))
> > + VERIFY(mr.number_of_active_allocations() == 1);
> > +
> > + for (auto _ : gen("const This& argument", std::allocator_arg , &mr))
> > + VERIFY(mr.number_of_active_allocations() == 1);
> > +
> > + VERIFY(mr.number_of_active_allocations() == 0);
> > +}
> > diff --git
> > a/libstdc++-v3/testsuite/24_iterators/range_generators/lwg3900.cc
> > b/libstdc++-v3/testsuite/24_iterators/range_generators/lwg3900.cc
> > new file mode 100644
> > index 00000000000..957879e8862
> > --- /dev/null
> > +++ b/libstdc++-v3/testsuite/24_iterators/range_generators/lwg3900.cc
> > @@ -0,0 +1,16 @@
> > +// { dg-do compile { target c++23 } }
> > +
> > +// LWG 3900. allocator_arg_t overloads of
> > generator::promise_type::operator new
> > +// should not be constrained
> > +
> > +#include <generator>
> > +#include <memory_resource>
> > +
> > +std::pmr::generator<int>
> > +bar(std::allocator_arg_t, std::pmr::memory_resource& mr) // { dg-error
> > "here" }
> > +{
> > + co_yield 3;
> > +}
> > +
> > +// { dg-error "static assertion failed" "" { target *-*-* } 0 }
> > +// { dg-error "no matching function .*memory_resource&" "" { target *-*-*
> > } 0 }
>
> --
> Arsen Arsenović
