This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [ranges] Improve ranges::{begin,end,cbegin,cend,data,empty,size,ssize}
Needs ReviewPublic

Authored by Quuxplusone on Dec 12 2021, 7:04 PM.

Details

Reviewers
ldionne
var-const
jloser
Group Reviewers
Restricted Project
Summary

This follows on from D115312, making all our CPOs follow the same pattern. In the process I found and fixed several conformance bugs (and probably fixed several more without finding them):

auto a = std::ranges::cbegin(std::string_view("x"));  // should be OK
auto b = std::ranges::cend(std::string_view("x"));  // should be OK
struct Foo { int size() &; };
int c = std::ranges::size(Foo());  // should be OK
struct Incomplete;
static_assert(not std::ranges::sized_range<Foo[]>);  // should be OK

I'm aware that the tests for all of these CPOs are bad, but I have no immediate plans to refactor them. I have added the "obviously" missing coverage for cbegin and cend, and added/corrected regression tests for the bugs mentioned above. I've also added test/libcxx/ tests for our diagnostic quality, which now is very good IMO.

If this looks good, the next step would be for me to implement the missing CPOs — cdata, rbegin, rend, crbegin, crend — along these same lines. I propose to split access.h into begin.h (begin/cbegin) and end.h (end/cend); introduce rbegin.h (rbegin/crbegin) and rend.h (rend/crend); and add cdata to the existing data.h.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Dec 12 2021, 7:04 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptDec 12 2021, 7:04 PM
jloser added inline comments.Dec 12 2021, 7:11 PM
libcxx/include/__ranges/data.h
58

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.

"Patch application failed" in buildkite. Rebase and try again.

libcxx/include/__ranges/access.h
146

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

libcxx/include/__ranges/data.h
58

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.

Fix and add regression tests for the SFINAE-friendliness issues noticed by @jloser — thanks!

After all that, I forgot to fix the contiguous_range<deque<int>> issue @jloser originally pointed at! Okay, now it's fixed.

Re-upload to poke buildkite, now that "patch application" finally succeeded on the parent revision D115312.

Quuxplusone marked an inline comment as done.Dec 13 2021, 11:34 AM

The GCC11 failures in https://reviews.llvm.org/harbormaster/unit/138963/ are due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101239 and I need to find workarounds. (I'm inclined to just change __t + 0 into _VSTD::__decay_copy(__t).)

Quuxplusone planned changes to this revision.Dec 15 2021, 7:28 PM

To be rebased after D115686 and D115838.

Quuxplusone retitled this revision from [libc++] [ranges] Improve ranges::{begin,end,cbegin,cend,data,size,ssize} to [libc++] [ranges] Improve ranges::{begin,end,cbegin,cend,data,empty,size,ssize}.
Quuxplusone edited the summary of this revision. (Show Details)

Rebase on the new-and-smaller D115312; and also on the already-landed improvements D115838 and D115686.
This PR is still a mix of "style improvements," "diagnostic improvements," and "actual conformance issues"; I'll continue trying to extricate the conformance issues into their own PRs, probably tomorrow.

jloser resigned from this revision.Mar 20 2023, 7:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2023, 7:42 PM
libcxx/test/libcxx/ranges/range.access/range.access.end/incomplete.verify.cpp