[libcxx-commits] [PATCH] D133638: [libc++] static_assert that rebinding the allocator works as expected

[Jordan Rupprecht via Phabricator via libcxx-commits]

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