[libcxx-commits] [libcxx] [libc++] Avoid -Wzero-as-null-pointer-constant in operator<=> (PR #79465)

[Louis Dionne via libcxx-commits]

================
@@ -30,14 +30,16 @@ class partial_ordering;
 class weak_ordering;
 class strong_ordering;
 
-template <class _Tp, class... _Args>
-inline constexpr bool __one_of_v = (is_same_v<_Tp, _Args> || ...);
-
 struct _CmpUnspecifiedParam {
-  _LIBCPP_HIDE_FROM_ABI constexpr _CmpUnspecifiedParam(int _CmpUnspecifiedParam::*) noexcept {}
-
-  template <class _Tp, class = enable_if_t<!__one_of_v<_Tp, int, partial_ordering, weak_ordering, strong_ordering>>>
-  _CmpUnspecifiedParam(_Tp) = delete;
+  // If anything other than a literal 0 is provided, the behavior is undefined by the Standard.
+  //
+  // We handle this by making this function consteval and making the program ill-formed if
+  // a value other than 0 is provided. This technically also accepts things that are not
+  // literal 0s like `1 - 1`. The alternative is to use the fact that a pointer can be
+  // constructed from literal 0, but this conflicts with `-Wzero-as-null-pointer-constant`.
+  template <class _Tp, class = __enable_if_t<is_same_v<_Tp, int> > >
+  _LIBCPP_HIDE_FROM_ABI consteval _CmpUnspecifiedParam(_Tp __zero) noexcept _LIBCPP_DIAGNOSE_ERROR(
+      __zero != 0, "Only literal 0 is allowed as the operand of a comparison with one of the ordering types") {}
----------------
ldionne wrote:

@cor3ntin This is really tricky. I can use `__diagnose_if__`, however in that case I really want to use `__enable_if` instead of `requires same_as`. Indeed, it seems that Clang evaluates those two "constraints" at different times, and the `_LIBCPP_DIAGNOSE_IF` condition (`__zero != 0`) is evaluated even when I pass something that's not an `int` if I use `requires same_as`.

So for example, I have a test case where I check that `strong_ordering == nullptr` doesn't compile (because there's no viable overload). If I use `requires same_as`, I also get a warning diagnostic because I am using 0 as a null pointer constant in the diagnose-if condition `__zero != 0`, where `__zero` is of type `nullptr_t`. That means I have to jump through hoops to either disable the diagnostic here or add an `expected-warning` in the test, but I don't want to do that since it's way too subtle.

Also, another point I want to raise about moving to `__diagnose_if__` is that we won't provide a diagnostic on GCC anymore. I think that's reasonable, but it's worth taking into account.

So IMO the patch in its current state goes for the best balance of simplicity, quality of implementation and maintainability.

https://github.com/llvm/llvm-project/pull/79465