[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 accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
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
----------------
Quuxplusone wrote:
> 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.
Yes, I noticed this is the same that you did in `std::strong_order`, and I wish I had caught that when the review for `strong_order` happened.

Ok, let's not block this for the release branch, however please go back and provide decent diagnostics for both `strong_order` (etc..) and these ones. I'm pretty serious about diagnostics not sucking, it's an important property for users. It's more important to provide good diagnostics to our thousands (?) of users than to have an implementation that looks nicer from our libc++ developer's perspective, when the two are within the same ballpark in terms of complexity.


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

https://reviews.llvm.org/D111514