[libcxx-commits] [PATCH] D118279: [libcxx][test] MaybePOCCAAllocator should meet the Cpp17Allocator requirements

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter added inline comments.


================
Comment at: libcxx/test/support/allocators.h:196-199
+    template <class U>
+    struct rebind {
+        typedef MaybePOCCAAllocator<U, POCCAValue> other;
+    };
----------------
Quuxplusone wrote:
> Isn't there allocator_traits magic that does this for all templates automatically? or is that only in pointer_traits?
The magic only works when the allocator is a specialization of a template with at least one type parameter and no non-type template parameters. ([allocator.traits.types]/11)


================
Comment at: libcxx/test/support/allocators.h:207
+    MaybePOCCAAllocator(const MaybePOCCAAllocator<U, POCCAValue>& that)
+        : id_(that.id()) {}
+
----------------
Quuxplusone wrote:
> If CI is happy with this, then OK, but I suspect that whatever is happening with `copy_assigned_into_` is not well-thought-out and works only by accident, wherever it's being used/tested.
I think I just had the constructor copy what the `==` examines for head-in-the-sand conformance, but I suspect the result is tests that always pass. I'll investigate.


================
Comment at: libcxx/test/support/allocators.h:209
+
+
     MaybePOCCAAllocator(const MaybePOCCAAllocator&) = default;
----------------
Unnecessary empty line.


================
Comment at: libcxx/test/support/allocators.h:231-232
 
-    friend bool operator==(const MaybePOCCAAllocator& lhs, const MaybePOCCAAllocator& rhs)
+    template <class U>
+    friend bool operator==(const MaybePOCCAAllocator& lhs, const MaybePOCCAAllocator<U, POCCAValue>& rhs)
     {
----------------
Quuxplusone wrote:
> I kneejerk want to leave this `operator==` alone and rely on the implicit converting ctor you just added... but would that make `allocT == allocU` ambiguous because the compiler doesn't know which operand to convert?
I think that would be ambiguous, but I don't claim to be able to reason about operator rewrites in the presence of implicit conversions. I'll investigate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118279