[libcxx-commits] [PATCH] D111514: [libc++] [P1614] Implement the second half of [cmp.alg]: compare_{strong, weak, partial}_fallback.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

The logic looks alright to me, but I have a significant comment about the approach taken and its impact on diagnostics.



================
Comment at: libcxx/include/__compare/compare_partial_order_fallback.h:31
+        template<class _Tp, class _Up>
+            requires is_same_v<decay_t<_Tp>, decay_t<_Up>>
+        _LIBCPP_HIDE_FROM_ABI static constexpr auto
----------------
I have already voiced concerns with this approach to constraining functions: it produces terrible diagnostics, and that's a bug. For example:

```
.../cmp/cmp.alg/compare_partial_order_fallback.pass.cpp:315:5: error: no matching function for call to object of type 'const std::__compare_partial_order_fallback::__fn'
    std::compare_partial_order_fallback(b, b);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/c++/v1/__compare/compare_partial_order_fallback.h:58:46: note: candidate template ignored: substitution failure [with _Tp = N12::B &, _Up = N12::B &]: no matching function for call to '__go'
        _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t, _Up&& __u) const
                                             ^
1 error generated.
```

That's it, nothing else. There's no backtrace at all, so it's impossible to figure out what we did wrong when calling the function. I don't know why it's so bad, I thought that even SFINAE-based failures had some backtrace, but it looks like that's not the case here.

I know I'm really annoying with this requirement, and I know it's painful and C++ doesn't give us good tools to achieve both correctness and good diagnostics. It's actually embarrassing to see how difficult it is to be both correct and user-friendly with concepts, when that was the whole point of the feature. I don't know whether that's a property of the feature itself or if it's just Clang doing a really bad job at merging SFINAE failures and concepts failures, but the result is the same, and it's terrible.

However, what I do know is that we don't want libc++ to become the "C++ Standard Library with crappy diagnostics". Clang and libc++ have always strived to provide a good user experience, and it's actually an important property to maintain. I don't care super strongly about how this is implemented, but I'd like to have reasonable diagnostics. Not even great, but something a user can work with.

The usual way to provide this is to flatten the constraints into a single `operator()` overload set. The constraints become more complicated because they need to be mutually exclusive, but it works and it gives decent diagnostics.


================
Comment at: libcxx/include/__compare/compare_partial_order_fallback.h:42
+        __go(_Tp&& __t, _Up&& __u, __priority_tag<0>)
+            noexcept(noexcept( _VSTD::forward<_Tp>(__t) == _VSTD::forward<_Up>(__u) ? partial_ordering::equivalent :
+                               _VSTD::forward<_Tp>(__t) < _VSTD::forward<_Up>(__u) ? partial_ordering::less :
----------------
And then align the rest.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/compare_partial_order_fallback.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Unless I missed it, I don't think we're testing that these are CPOs anywhere. Can we add tests for this? Perhaps they can be grouped similarly to what we're doing for ranges.


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

https://reviews.llvm.org/D111514