[libcxx-commits] [PATCH] D116239: [libc++] [ranges] ADL-proof the [range.access] CPOs.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/access.h:55
     __can_borrow<_Tp> &&
     __class_or_enum<remove_cvref_t<_Tp>> &&
     requires(_Tp && __t) {
----------------
ldionne wrote:
> Quuxplusone wrote:
> > Quuxplusone wrote:
> > > @jloser writes:
> > > > Regarding the class_or_enum discussion, note `__unqualified_begin` uses `__class_or_enum`, so we should change that (and others, where appropriate)
> > > 
> > > For `__unqualified_begin`, `__class_or_enum` is both sufficient //and// necessary: it's looking for `begin(__t)`, which means it //needs// to do ADL even for enums (but, again, not for pointers).
> > > 
> > > I still don't understand why the well-formedness check of `t.begin()` triggers instantiation of `Holder<Incomplete>` when `t` is a pointer ( https://godbolt.org/z/Tfb3vTrvf ); I should ask around about that.
> > @ldionne wrote:
> > > This looks like a Clang bug to me, is that possible? I don't understand why t.begin() could ever trigger ADL, and hence why this fix is necessary from the library side at all. Instead, I would expect that we fix Clang and then add the tests.
> > 
> > Yes, it's a Clang bug — and I just tracked it down and filed it as https://github.com/llvm/llvm-project/issues/52970 . However, even if we can get Clang14 fixed, we still need this as a workaround since we want libc++14 to work with Clang13.
> > Yes, it's a Clang bug — and I just tracked it down and filed it as https://github.com/llvm/llvm-project/issues/52970 .
> 
> Woooh, I love when we do that! Thanks for looking into it!
> 
> In that case, I would suggest that we add this:
> 
> ```
> template <class _Tp>
> concept __workaround_52970 = __class_or_enum<remove_cvref_t<_Tp>>;
> ```
> 
> Along with a suitable comment, and then we can use `__workaround_52970<_Tp>` in the affected concepts. Then, as soon as we drop support for Clang 13, we can grep-remove this workaround. WDYT?
Sure, works for me. (This is another opportunity to open the can of worms of whether it should be, like, `concept __workaround_52970 = is_class_v<remove_cvref_t<_Tp>> || is_union_v<remove_cvref_t<_Tp>>;` — but I'm okay not opening that can.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116239