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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

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`.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133638/new/

https://reviews.llvm.org/D133638