[libcxx-commits] [PATCH] D115607: [libc++] [ranges] Improve ranges::{begin, end, cbegin, cend, data, size, ssize}

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/access.h:146
+      requires sentinel_for<decltype(_VSTD::__decay_copy(end(__t))), iterator_t<remove_reference_t<_Tp>>>
+      { return          _VSTD::__decay_copy(end(__t)); }
 
----------------
Here `__go` is constrained with `requires __class_or_enum<...>`, and has a dependent return type involving ADL `end`. I expect this to //not be ADL-proof// for suitably evil inputs, because dependent-return-type-SFINAE stuff happens before constraint satisfaction (AIUI, and/or at least on Clang it seemed to).

...Yep, here's the dynamite. https://godbolt.org/z/j453na433
This case fails both before //and// after D115607. Here's the failure mode after:
```
x.cpp:4:37: error: field has incomplete type 'Incomplete'
template<class T> struct Holder { T t; };
                                    ^
__ranges/access.h:39:51: note: in instantiation of template class 'Holder<Incomplete>' requested here
    requires (_Tp __t) { { _VSTD::__decay_copy(__t.begin()) } -> input_or_output_iterator; } ||
                                                  ^
__ranges/access.h:39:28: note: in instantiation of requirement here
    requires (_Tp __t) { { _VSTD::__decay_copy(__t.begin()) } -> input_or_output_iterator; } ||
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
__ranges/access.h:39:5: note: while substituting template arguments into constraint expression here
    requires (_Tp __t) { { _VSTD::__decay_copy(__t.begin()) } -> input_or_output_iterator; } ||

__ranges/access.h:80:16: note: while checking the satisfaction of concept '__basic_requirements<Holder<Incomplete> *&>' requested here
      requires __basic_requirements<_Tp&&> &&
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
```
Since this one fails before //and// after, it's not a regression per se. However, it's also easy to fix using `enable_if_t`, as I've just proved to myself locally, so I might as well upload the fix. :)

Actually the part depicted here is also a Clang bug: https://github.com/llvm/llvm-project/issues/52643


================
Comment at: libcxx/include/__ranges/data.h:58
+      noexcept(noexcept(_VSTD::to_address(ranges::begin(__t))))
+      -> decltype(      _VSTD::to_address(ranges::begin(__t)))
+      requires contiguous_iterator<decltype(ranges::begin(__t))>
----------------
jloser wrote:
> Is it safe to use `to_address` here in trailing return type SFINAE? Remember `to_address` is **not** SFINAE-friendly as we've talked about in other PRs.
Ah, this reminds me that I'm doing something in a couple places here that I //didn't// actually expect to work (but then it passed all the existing tests so I forgot about it).
Here `__go` is constrained with `requires contiguous_iterator<...>`, and has a dependent return type involving `to_address`. I expect this to //not work// for suitably evil inputs, because dependent-return-type-SFINAE stuff happens before constraint satisfaction (AIUI, and/or at least on Clang it seemed to).
...Yep.
```
static_assert(!std::invocable<decltype(std::ranges::data), std::deque<int>&>);  // now a hard error, oops
```
This is now fixed, too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115607