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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/access.h:55
     __can_borrow<_Tp> &&
     __class_or_enum<remove_cvref_t<_Tp>> &&
     requires(_Tp && __t) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116239