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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
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
----------------
ldionne wrote:
> 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.
I have a proposed solution for this in D115607, but won't have time to revisit it before release/14.x.
What I'm doing in D111514 is the same thing I did in `std::strong_order` etc., so we are consistent between `std::strong_order` and `std::compare_strong_order_fallback`. I'm willing (even interested) to revisit these diagnostics post-14.x, but I still think there's value in getting all of [cmp.alg] shipped before the release branch happens.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.alg/compare_partial_order_fallback.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> 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.
Ack. I forgot to uncomment the now-existing CPO tests in `cpo.compile.pass.cpp`. Fixed.


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

https://reviews.llvm.org/D111514