[libcxx-commits] [PATCH] D115312: [libc++] [ranges] Simplify and fix a bug in ranges::empty.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/empty.h:61
   };
 }
 
----------------
Quuxplusone wrote:
> >>! @ldionne wrote:
> > I don't especially like the fact that we're moving away from requires clauses and back to straight SFINAE. Can you check what effect this has on diagnostics?
> 
> Old: 32 lines; new: 8 lines. I would call the new version "frustratingly opaque" but the old version "annoyingly verbose."
> https://godbolt.org/z/GseTh84jx
> I can't think of any way to improve this. I thought of constraining `operator()` on `requires range<decay_t<_Tp>>`, but (1) `ranges::empty(e)` //doesn't// require `e` to satisfy `range` (if it provides `.empty()`), and (2) that doesn't help because SFINAE kicks in prior to constraint checking, and (3) the constraint doesn't help the user anyway because the failure case 99% of the time will be that `e` //is// a range but can't be checked for emptiness for some other obscure reason.
Honestly, the output after is pretty terrible. It doesn't show anything at all, so it's basically impossible to know what's going wrong. I don't think we want to use this new technique in our ranges implementation. I would rather we revert to the more verbose and arguably not so great technique we had before, where we have to manually exclude other overloads.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115312