[libcxx-commits] [PATCH] D133638: [libc++] static_assert that rebinding the allocator works as expected
Jordan Rupprecht via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Oct 12 08:19:13 PDT 2022
rupprecht added a comment.
In D133638#3852135 <https://reviews.llvm.org/D133638#3852135>, @philnik wrote:
> In D133638#3850608 <https://reviews.llvm.org/D133638#3850608>, @rupprecht wrote:
>
>> 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?
>
> 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 `Foo`s 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`.
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!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133638/new/
https://reviews.llvm.org/D133638
More information about the libcxx-commits
mailing list