Details
- Reviewers
ldionne Mordante var-const huixie90 - Group Reviewers
Restricted Project - Commits
- rGd7d586e5a7d7: [libc++] static_assert that rebinding the allocator works as expected
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Just out of curiosity, can you link to the place in the standard where that requirement is laid out? I can't imagine things working reliably if that's not satisfied, but it would be nice to back this with something solid. And in that case, probably reference that section in the static assert message.
I'm seeing some build failures that don't seem to be the intended effect of this patch:
allocator_traits.h:159:57: error: no member named 'rebind' in 'FooAllocator'
using type _LIBCPP_NODEBUG = typename _Tp::template rebind<_Up>::other;
~~~~~ ^
allocator_traits.h:172:1: note: in instantiation of template class 'std::__allocator_traits_rebind<FooAllocator, Foo, false>' requested here
using __allocator_traits_rebind_t = typename __allocator_traits_rebind<_Alloc, _Tp>::type;
^
allocator_traits.h:244:5: note: in instantiation of template type alias '__allocator_traits_rebind_t' requested here
using rebind_alloc = __allocator_traits_rebind_t<allocator_type, _Tp>;
^
allocator_traits.h:352:1: note: in instantiation of template type alias 'rebind_alloc' requested here
using __rebind_alloc _LIBCPP_NODEBUG = typename _Traits::template rebind_alloc<_Tp>;
^
vector:361:43: note: in instantiation of template type alias '__rebind_alloc' requested here
static_assert(is_same<allocator_type, __rebind_alloc<__alloc_traits, value_type> >::value,
^
foo.h:71:17: note: in instantiation of template class 'std::vector<Foo, FooAllocator>' requested here
Foos foos_;With the allocator definition being:
class FooAllocator {
public:
using value_type = Foo;
FooAllocator() = default;
Foo* allocate(size_t num_objects);
void deallocate(Foo* ptr, size_t num_objects);
bool operator==(const FooAllocator&) const { return true; }
bool operator!=(const FooAllocator&) const { return false; }
};
using Foos = std::vector<Foo, FooAllocator>;
Foos foos_;(obviously sanitized)
It seems this patch static_asserts that rebind works as expected, but in this case, FooAllocator does not implement rebind at all -- which seems to be expected to work, per https://eel.is/c++draft/allocator.requirements#general-98. And the build failure seems to be due to rebind not being implemented.
As a standalone repro: https://godbolt.org/z/z64438qn7 (godbolt has not caught up, but that fails locally)
Shall I revert, or is the fix obvious?
@rupprecht I don't think there is a bug here. You allocator isn't conforming. The difference between your example and the standard example is that the standard example is a template, which can be rebound by default (see https://eel.is/c++draft/allocator.requirements#general-18). This is actually one of the bugs this patch is trying to catch.
I see. That's a surprising result of this patch, and the error message is not very clear. Is there any way a static assert could be added to make it more clear?
Would this be valid way to fix up any breakages with minimal changes? It builds, but will this just hit some other conformance check later?
#include <vector>
struct Foo {
int f;
};
template <typename T = Foo> // <-- Add this
class FooAllocator {
public:
static_assert(std::is_same_v<T, Foo>, "T must be Foo"); // <-- Add this (if desired)
using value_type = Foo;
FooAllocator() = default;
Foo* allocate(size_t num_objects);
void deallocate(Foo* ptr, size_t num_objects);
bool operator==(const FooAllocator&) const { return true; }
bool operator!=(const FooAllocator&) const { return false; }
};
void x() { std::vector<Foo, FooAllocator<>> y; } // <-- FooAllocator is now FooAllocator<>To clarify, it would be nice if this had a static assert like "allocator must be a template or explicitly implement rebind". Encountering "no member named rebind" is not at all related to the fix that needs to be made.
I'll look into that. Maybe it's possible to generate a nicer error when you try to rebind an allocator which doesn't support rebinding.
Would this be valid way to fix up any breakages with minimal changes? It builds, but will this just hit some other conformance check later?
#include <vector> struct Foo { int f; }; template <typename T = Foo> // <-- Add this class FooAllocator { public: static_assert(std::is_same_v<T, Foo>, "T must be Foo"); // <-- Add this (if desired) using value_type = Foo; FooAllocator() = default; Foo* allocate(size_t num_objects); void deallocate(Foo* ptr, size_t num_objects); bool operator==(const FooAllocator&) const { return true; } bool operator!=(const FooAllocator&) const { return false; } }; void x() { std::vector<Foo, FooAllocator<>> y; } // <-- FooAllocator is now FooAllocator<>
It's still not conforming, since the containers are allowed to rebind the allocator to any type. So I wouldn't guarantee that we will never add a check to catch this kind of allocator. For the short term it would probably work, but my recommendation would be to
- Implement the allocator in a type-agnostic way, i.e. replace any Foos with T, or
- Look into using polymorphic_allocator and implement a memory_resource
That would allow you to use the allocator more generally, instead of for a specific type. I of course don't know exactly what your allocator does and what environment you are deploying in, so these options might not be viable for you.
As a temporary work-around it might also be easier to add something like
template <class T> struct rebind { // TODO: Allow rebinding the allocator properly static_assert(std::is_same_v<T, Foo>, "Rebinding this allocator to anything other than Foo does not work currently"); using other = FooAllocator; };
to FooAllocator.
We could also look into changing it into a warning for the containers that don't actually rebind the allocator if this breaks too many people.
I agree with @philnik 's diagnostic here but I'd like to strongly +1 the request to improve the error message. Perhaps we should change this to static_assert(__check_allocator_requirements<allocator_type>, ""); and then have __check_allocator_requirements validate a few properties of the allocator, including that it has rebind, and then that such rebind behaves as expected.
This allocator isn't doing anything interesting with the Foo type, so I think we can make it a normal templated allocator. But having this rebind workaround helps if we encounter stranger allocators while rolling this change out.
We could also look into changing it into a warning for the containers that don't actually rebind the allocator if this breaks too many people.
I have no idea how many things this will affect for us, but I (or someone on my team) can follow up in a week-ish, give or take, if this has a widespread impact.
Thanks for all the help!
I have a followup question regarding this: the statement you linked (https://eel.is/c++draft/allocator.requirements#general-18) says that for SomeAllocator<T, Args> if "Allocator does not supply a rebind member template, the standard allocator_traits template uses SomeAllocator<U, Args> in place of Allocator::rebind<U>::other by default."
With this change the statement stands for a template <class T, class Y> class FooAllocator {.. } (https://godbolt.org/z/4PjeedeP4) and doesn't for a template <class T, int X> class FooAllocator {.. } (https://godbolt.org/z/j9sxo65vc).
In the latter case we still look for the rebind<_Up>::other member template.
Is that intended?
Yes. I don't think there is a way to generally rebind classes with NTTPs. This has to be special-cased for these allocators.