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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM as long as the answers to all my questions are "no, what I have written here is correct." ;)



================
Comment at: libcxx/test/support/allocators.h:196-199
+    template <class U>
+    struct rebind {
+        typedef MaybePOCCAAllocator<U, POCCAValue> other;
+    };
----------------
Isn't there allocator_traits magic that does this for all templates automatically? or is that only in pointer_traits?


================
Comment at: libcxx/test/support/allocators.h:207
+    MaybePOCCAAllocator(const MaybePOCCAAllocator<U, POCCAValue>& that)
+        : id_(that.id()) {}
+
----------------
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.


================
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)
     {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118279