llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
26.71k stars 10.94k forks source link

"Unspecified" parameter for the standard comparison category types' comparison operators might be overconstrained #96237

Open dangelog opened 1 week ago

dangelog commented 1 week ago

Hi,

Consider https://gcc.godbolt.org/z/ndrvzE8j1

#include <compare>

struct P {
    operator std::partial_ordering() const;
};

int main() {
    std::partial_ordering::equivalent == P{};
}

Clang + libc++ rejects this with:

<source>:8:39: error: use of overloaded operator '==' is ambiguous (with operand types 'const partial_ordering' and 'P')
    8 |     std::partial_ordering::equivalent == P{};
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~
/opt/compiler-explorer/clang-trunk-20240620/bin/../include/c++/v1/__compare/ordering.h:62:47: note: candidate function
   62 |   _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(partial_ordering, partial_ordering) noexcept = default;
      |                                               ^
/opt/compiler-explorer/clang-trunk-20240620/bin/../include/c++/v1/__compare/ordering.h:64:47: note: candidate function
   64 |   _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
      |                                               ^
1 error generated.
Compiler returned: 1

It's not entirely clear why this should be rejected, instead of using the friend constexpr bool operator==(partial_ordering v, partial_ordering w) noexcept = default; operator described by https://eel.is/c++draft/cmp .

https://eel.is/c++draft/cmp#categories.pre-3 is quite ambiguous about whether the code above should be accepted, or if it falls under the "In this context, the behavior of a program that supplies an argument other than a literal 0 is undefined" https://eel.is/c++draft/cmp#categories.pre-3.sentence-4 sentence.

I am not sure what "in this context" means: does it mean that passing P{} is UB because it's not a literal 0? That sounds like a vexing interpretation: the very same overload is selected by e.g. std::partial_ordering::equivalent == std::strong_ordering::equivalent after converting strong_ordering, and I don't see a provision that would allow this.

Ultimately it boils down to the SFINAE on _CmpUnspecifiedParam's constructor here: https://github.com/llvm/llvm-project/blob/main/libcxx/include/__compare/ordering.h#L39 , which looks "poisoned". It excludes the standard comparison types, but not types convertible to those. It may also break user-defined comparisons between standard category types and user-defined types (not convertible to anything), like here: https://gcc.godbolt.org/z/3aPedMWoe

Related:


For some more background: in Qt we have defined our own Qt::*_ordering types because Qt only requires C++17. Basically, we've backported the standard comparison types. We want them to be interoperable with the standard ones, so they define constructors/conversion operators towards the standard types. Which leads to the problem shown by the testcase above (P is Qt::partial_ordering), tracked by https://bugreports.qt.io/browse/QTBUG-126541 .

llvmbot commented 1 week ago

@llvm/issue-subscribers-clang-frontend

Author: Giuseppe D'Angelo (dangelog)

Hi, Consider https://gcc.godbolt.org/z/ndrvzE8j1 ``` #include <compare> struct P { operator std::partial_ordering() const; }; int main() { std::partial_ordering::equivalent == P{}; } ``` Clang + libc++ rejects this with: ``` <source>:8:39: error: use of overloaded operator '==' is ambiguous (with operand types 'const partial_ordering' and 'P') 8 | std::partial_ordering::equivalent == P{}; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~ /opt/compiler-explorer/clang-trunk-20240620/bin/../include/c++/v1/__compare/ordering.h:62:47: note: candidate function 62 | _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(partial_ordering, partial_ordering) noexcept = default; | ^ /opt/compiler-explorer/clang-trunk-20240620/bin/../include/c++/v1/__compare/ordering.h:64:47: note: candidate function 64 | _LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(partial_ordering __v, _CmpUnspecifiedParam) noexcept { | ^ 1 error generated. Compiler returned: 1 ``` It's not entirely clear why this should be rejected, instead of using the `friend constexpr bool operator==(partial_ordering v, partial_ordering w) noexcept = default;` operator described by https://eel.is/c++draft/cmp . https://eel.is/c++draft/cmp#categories.pre-3 is quite ambiguous about whether the code above should be accepted, or if it falls under the "In this context, the behavior of a program that supplies an argument other than a literal 0 is undefined" https://eel.is/c++draft/cmp#categories.pre-3.sentence-4 sentence. I am not sure what "in this context" means: does it mean that passing `P{}` is UB because it's not a literal 0? That sounds like a vexing interpretation: the very same overload is selected by e.g. `std::partial_ordering::equivalent == std::strong_ordering::equivalent` after converting `strong_ordering`, and I don't see a provision that would _allow_ this. Ultimately it boils down to the SFINAE on `_CmpUnspecifiedParam`'s constructor here: https://github.com/llvm/llvm-project/blob/main/libcxx/include/__compare/ordering.h#L39 , which looks "poisoned". It excludes the standard comparison types, but not types *convertible* to those. It may also break user-defined comparisons between standard category types and user-defined types (not convertible to anything), like here: https://gcc.godbolt.org/z/3aPedMWoe Related: * https://cplusplus.github.io/LWG/issue4051 * https://github.com/llvm/llvm-project/issues/43670 * https://github.com/llvm/llvm-project/pull/79465 --- For some more background: in Qt we have defined our own `Qt::*_ordering` types because Qt only requires C++17. Basically, we've backported the standard comparison types. We want them to be interoperable with the standard ones, so they define constructors/conversion operators towards the standard types. Which leads to the problem shown by the testcase above (`P` is `Qt::partial_ordering`), tracked by https://bugreports.qt.io/browse/QTBUG-126541 .