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

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter marked 2 inline comments as done.
CaseyCarter added inline comments.


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


================
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)
     {
----------------
CaseyCarter wrote:
> 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.
At least two compilers agree with us that it would be ambigous.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118279