[libc++] Further constrain comparison against foo_ordering types

[ldionne]

This fixes an issue reported in #79465.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This fixes an issue reported in #79465.

---

Full diff: https://github.com/llvm/llvm-project/pull/127311.diff

2 Files Affected:

• (modified) libcxx/include/__compare/ordering.h (+5)
• (modified) libcxx/test/std/language.support/cmp/cmp.categories.pre/reject-other-than-literal-zero.verify.cpp (+39-8)

diff --git a/libcxx/include/__compare/ordering.h b/libcxx/include/__compare/ordering.h
index 902ef5329dd43..d8fffeda40000 100644
--- a/libcxx/include/__compare/ordering.h
+++ b/libcxx/include/__compare/ordering.h
@@ -49,6 +49,11 @@ struct _CmpUnspecifiedParam {
   {
     (void)__zero;
   }
+
+  // Reject any other type and reject int lvalues.
+  template <class T>
+  _CmpUnspecifiedParam(T&&)                 = delete;
+  _CmpUnspecifiedParam(const volatile int&) = delete;
 };
 
 class partial_ordering {
diff --git a/libcxx/test/std/language.support/cmp/cmp.categories.pre/reject-other-than-literal-zero.verify.cpp b/libcxx/test/std/language.support/cmp/cmp.categories.pre/reject-other-than-literal-zero.verify.cpp
index b6bc4dd4f097a..d86bc418685e2 100644
--- a/libcxx/test/std/language.support/cmp/cmp.categories.pre/reject-other-than-literal-zero.verify.cpp
+++ b/libcxx/test/std/language.support/cmp/cmp.categories.pre/reject-other-than-literal-zero.verify.cpp
@@ -22,22 +22,33 @@
 
 #include "test_macros.h"
 
+struct AnyType {
+  operator int() const { return 0; }
+};
+
 #define TEST_FAIL(v, op)                                                                                               \
   do {                                                                                                                 \
     /* invalid types */                                                                                                \
+    constexpr AnyType t;                                                                                               \
     void(v op 0L);                                                                                                     \
     void(0L op v);                                                                                                     \
     void(v op 0.0);                                                                                                    \
     void(0.0 op v);                                                                                                    \
     void(v op nullptr);                                                                                                \
     void(nullptr op v);                                                                                                \
+    void(v op t);                                                                                                      \
+    void(t op v);                                                                                                      \
     /* invalid value */                                                                                                \
     void(v op 1);                                                                                                      \
     void(1 op v);                                                                                                      \
-    /* value not known at compile-time */                                                                              \
+    /* lvalue reference (also, value is not known at compile-time) */                                                  \
     int i = 0;                                                                                                         \
     void(v op i);                                                                                                      \
     void(i op v);                                                                                                      \
+    /* value known at compile time, but still a lvalue */                                                              \
+    constexpr int j = 0;                                                                                               \
+    void(v op j);                                                                                                      \
+    void(j op v);                                                                                                      \
   } while (false)
 
 #define TEST_PASS(v, op)                                                                                               \
@@ -50,13 +61,33 @@
 
 template <typename T>
 void test_category(T v) {
-  TEST_FAIL(v, ==);  // expected-error 30 {{invalid operands to binary expression}}
-  TEST_FAIL(v, !=);  // expected-error 30 {{invalid operands to binary expression}}
-  TEST_FAIL(v, <);   // expected-error 30 {{invalid operands to binary expression}}
-  TEST_FAIL(v, <=);  // expected-error 30 {{invalid operands to binary expression}}
-  TEST_FAIL(v, >);   // expected-error 30 {{invalid operands to binary expression}}
-  TEST_FAIL(v, >=);  // expected-error 30 {{invalid operands to binary expression}}
-  TEST_FAIL(v, <=>); // expected-error 30 {{invalid operands to binary expression}}
+  TEST_FAIL(v, ==);
+  // expected-error-re@-1 36 {{conversion function from '{{.+}}' to '_CmpUnspecifiedParam' invokes a deleted function}}
+  // expected-error-re@-2 6 {{conversion from '{{.+}}' to '_CmpUnspecifiedParam' is ambiguous}}
+
+  TEST_FAIL(v, !=);
+  // expected-error-re@-1 36 {{conversion function from '{{.+}}' to '_CmpUnspecifiedParam' invokes a deleted function}}
+  // expected-error-re@-2 6 {{conversion from '{{.+}}' to '_CmpUnspecifiedParam' is ambiguous}}
+
+  TEST_FAIL(v, <);
+  // expected-error-re@-1 36 {{conversion function from '{{.+}}' to '_CmpUnspecifiedParam' invokes a deleted function}}
+  // expected-error-re@-2 6 {{conversion from '{{.+}}' to '_CmpUnspecifiedParam' is ambiguous}}
+
+  TEST_FAIL(v, <=);
+  // expected-error-re@-1 36 {{conversion function from '{{.+}}' to '_CmpUnspecifiedParam' invokes a deleted function}}
+  // expected-error-re@-2 6 {{conversion from '{{.+}}' to '_CmpUnspecifiedParam' is ambiguous}}
+
+  TEST_FAIL(v, >);
+  // expected-error-re@-1 36 {{conversion function from '{{.+}}' to '_CmpUnspecifiedParam' invokes a deleted function}}
+  // expected-error-re@-2 6 {{conversion from '{{.+}}' to '_CmpUnspecifiedParam' is ambiguous}}
+
+  TEST_FAIL(v, >=);
+  // expected-error-re@-1 36 {{conversion function from '{{.+}}' to '_CmpUnspecifiedParam' invokes a deleted function}}
+  // expected-error-re@-2 6 {{conversion from '{{.+}}' to '_CmpUnspecifiedParam' is ambiguous}}
+
+  TEST_FAIL(v, <=>);
+  // expected-error-re@-1 36 {{conversion function from '{{.+}}' to '_CmpUnspecifiedParam' invokes a deleted function}}
+  // expected-error-re@-2 6 {{conversion from '{{.+}}' to '_CmpUnspecifiedParam' is ambiguous}}
 
   TEST_PASS(v, ==);
   TEST_PASS(v, !=);

[ldionne]

@zygoloid I'm not super thrilled by the change in diagnostics but I think I can live with it.

[ldionne]

Hmm, this doesn't seem to work (seen in CI):

In file included from <...>/libcxx/test/std/utilities/function.objects/refwrap/refwrap.comparissons/compare.three_way.refwrap.const_ref.pass.cpp:23:
<...>/libcxx/test/support/test_comparisons.h:167:23: error: use of overloaded operator '==' is ambiguous (with operand types 'std::strong_ordering' and 'std::weak_ordering')
  167 |     return (t1 <=> t2 == order) && testComparisonsComplete(t1, t2, equal, less, greater);
      |             ~~~~~~~~~ ^  ~~~~~
<...>/libcxx/test/std/utilities/function.objects/refwrap/refwrap.comparissons/compare.three_way.refwrap.const_ref.pass.cpp:51:12: note: in instantiation of function template specialization 'testOrder<std::weak_ordering, std::reference_wrapper<int>, int>' requested here
   51 |     assert(testOrder(rw1, t, Order::equivalent));
      |            ^
<...>/libcxx/test/std/utilities/function.objects/refwrap/refwrap.comparissons/compare.three_way.refwrap.const_ref.pass.cpp:73:3: note: in instantiation of function template specialization 'test<int, std::weak_ordering>' requested here
   73 |   test<int, std::weak_ordering>();
      |   ^
<...>/build/generic-modules/libcxx/test-suite-install/include/c++/v1/__compare/ordering.h:221:47: note: candidate function
  221 |   _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
      |                                               ^
<...>/build/generic-modules/libcxx/test-suite-install/include/c++/v1/__compare/ordering.h:143:47: note: candidate function
  143 |   _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(weak_ordering, weak_ordering) noexcept = default;
      |                                               ^
<...>/build/generic-modules/libcxx/test-suite-install/include/c++/v1/__compare/ordering.h:145:47: note: candidate function (with reversed parameter order)
  145 |   _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
      |                                               ^

I think that's because = delete on the _CmpUnspecifiedParam constructors is actually different from not providing constructors at all -- it defines them but defines them as deleted, which means they are still candidates for overload resolution. @zygoloid Given this, I'm not certain how to move forward with this patch. I'm personally not very inconvenienced by the fact that we accept x <=> y == something_constexpr, so I'd be tempted to just leave it as-is unless you have a suggestion for moving forward.

[zygoloid]

My preference would be to add an attribute or builtin type to Clang so that libc++ can use
bool operator==(strong_ordering __v, [[clang::zero_literal_param]] int)
or
bool operator==(strong_ordering __v, __zero_literal_type)
In addition to accepting the exact right set of cases, we ought to be able to issue much better diagnostics that way too.