Page MenuHomePhabricator

[libcxx][ranges] Add range.subrange.
ClosedPublic

Authored by zoecarver on May 6 2021, 10:15 AM.

Details

Summary

Basically the title.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
zoecarver marked 6 inline comments as done.Tue, May 25, 3:32 PM
zoecarver added inline comments.
libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp
30 ↗(On Diff #344990)

Thanks :)

zoecarver updated this revision to Diff 347809.Tue, May 25, 3:33 PM
  • Rebase.
  • Address all comments.
Mordante accepted this revision as: Mordante.Wed, May 26, 8:37 AM

LGTM, except for the [[nodiscard]] on empty(). Please make sure the build passes CI before committing.

libcxx/include/__ranges/subrange.h
160

This one shouldn't be [[nodiscard]] according to the Standard.

Sorry for the slow reply. Thanks @Mordante.

@cjdb how's this look now?

libcxx/include/__ranges/subrange.h
160

I think we decided that members named "empty" are sufficiently vexing because users something think it will empty the container (or in this case sub range).

zoecarver updated this revision to Diff 349129.Tue, Jun 1, 4:32 PM

Rebase.

Note: I'm going to land this when the tests pass.

Mordante added inline comments.Wed, Jun 2, 10:14 AM
libcxx/include/__ranges/subrange.h
160

Oke wasn't aware we decided that. But I agree calling size() and discarding the result is most likely a bug.

ldionne requested changes to this revision.Wed, Jun 2, 1:11 PM

General note for the tests - can we split tests so that we test one overload (as documented in the Standard) per file? This is what we do elsewhere and it makes it easier to make sure we're testing everything correctly. If a function is only a very tiny derivative of another one, then it's OK to test both in one shot, of course.

libcxx/include/__ranges/subrange.h
47

This is *so* weird - I would have expected something like tuple_size<_Tp>::value == 2 instead. But leave as-is, that's how the Standard does it.

65

For a static like that, I'd use _StoreSize instead.

Re: using _Iter instead of _Tp, I strongly agree -- using _Iter is a lot clearer than what the Standard uses.

66

__begin_? (same for other member variables)

75

Here, we're going to store the size if _Kind == subrange_kind::sized even if _Sent is a sized sentinel for _Iter. We should not be storing it when _Sent is a sized sentinel.

98–100

Instead, you could use this->member when referring to a member in the dependent base, and _Base::__store_size when referring to the static. IMO that's easier to follow and more idiomatic, WDYT?

143

I'm surprised they didn't make this _Iter begin() && instead to make sure we can only call this on a xvalue.

149

See, I'm very curious to understand why the Standard didn't mark this one as [[nodiscard]]. It can't be an oversight, since they marked a bunch of methods around it [[nodiscard]]. Just curious to understand what the motivation was.

libcxx/test/std/ranges/range.utility/range.subrange/access.pass.cpp
88 ↗(On Diff #349129)

testPrimitives?

libcxx/test/std/ranges/range.utility/range.subrange/ctad.compile.pass.cpp
22

namespace ranges = std::ranges or something like that, but we frown upon importing names.

This revision now requires changes to proceed.Wed, Jun 2, 1:11 PM
zoecarver added inline comments.Wed, Jun 2, 2:02 PM
libcxx/include/__ranges/subrange.h
65

The style I try to use is that variables are always __snake_case or camelCase (regardless of staticness) and the former at least seems to be the existing style. This way, _TitleCase or TitleCase is reserved for types (also matches current style and is my personal preference).

66

In recent commits, at least, I tend not to suffix with an underscore. Historically in libc++ we seem to do both. If you feel strongly, let's add some docs/style guid and I'll fix it.

81

Nit: no constexpr.

98–100

This is all personal preference, but the way I see it, there's no confusion as to what __begin is referring to and it's unconditionally a member. If someone went looking for the definition, they'd find this which would lead them to it.

In fact, there might be an argument that saying this->member adds confusion, because someone might say "why do we need to use this->" or try to remove that.

For me it really comes down to simplicity/brevity. Writing __begin is simply fewer characters.

143

This is odd and does not seem to be inline with most views.

Quuxplusone added inline comments.Wed, Jun 2, 8:38 PM
libcxx/include/__ranges/subrange.h
50–51

Check whether these should be ADL or std::get, and then (if the ADL is intentional) let's please figure out a way to document that in the code long-term.
I'm almost amenable to making a macro like #define _LIBCPP_ADL(x) x just to tag intentional ADLs. That'd be a bigger change than this PR, obviously. For this PR, if the ADL is intentional, I think it'd suffice to just leave a comment // intentional ADL.

66

FWIW, I feel strongly: libc++ should follow libc++ style, which is "data members get underscore-suffixed."

98–100

FWIW, I agree with @ldionne: using-declarations (and their effect on name lookup) are significantly more confusing for the reader/maintainer than the plain old this->__member_ expression syntax we're all familiar with.

143

Tangential, but yeah: it wouldn't make sense to require the caller to write std::move(myRange).begin(), because then how would you ever get at the end-sentinel? You couldn't write again std::move(myRange).end(), because that would trigger all your static-analyzer's "double-moved-from" checks.

184

this->__size_ += _VSTD::__to_unsigned_like(-__n);
(The ADL-proofing of __to_unsigned_like actually doesn't matter because the lookup of operator- will be ADL anyway; but let's be consistent in our "ADL-proof by default" style.)
Ditto line 190 below.

Orthogonally: Is the whole point of __store_size_ just to keep track of whether __size_ exists? Could we just do

if constexpr (requires { this->__size_; })
    this->__size_ += ...;

instead, throughout? Or would that not-work for some technical reason?

209–210

Formatting nit: the -> belongs on the second line. Lines 199-200 above do it right AFAIC.

libcxx/test/std/ranges/range.utility/range.subrange/access.pass.cpp
58 ↗(On Diff #349129)

Definitely just write out std::ptrdiff_t in the like two places it appears.

73 ↗(On Diff #349129)

Formatting nit: friend constexpr

libcxx/test/std/ranges/range.utility/range.subrange/ctad.compile.pass.cpp
22

(moving my comment from the previous file after seeing Louis's)
Looking through this test file, I don't think skipping the std:: on lines 89–163 is really buying you anything. Strongly consider just writing out std::ranges::whatever everywhere it needs to be. (The benefit is greppability.)

27

One way to clean this up is to introduce an array of int a and a typedef FI:

int a[10];
using FI = forward_iterator<int*>;
static_assert(std::same_as<decltype(subrange(FI(a), FI(a))),
    subrange<FI, FI, std::ranges::subrange_kind::unsized>>);
static_assert(std::same_as<decltype(subrange(FI(a), FI(a), 0)),
    subrange<FI, FI, std::ranges::subrange_kind::sized>>);
static_assert(std::same_as<decltype(subrange(a, a)),
    subrange<int*, int*, std::ranges::subrange_kind::unsized>>);
static_assert(std::same_as<decltype(subrange(a, a, 0)),
    subrange<int*, int*, std::ranges::subrange_kind::sized>>);
static_assert(std::same_as<decltype(subrange(a, nullptr)),
    subrange<int*, std::nullptr_t, std::ranges::subrange_kind::unsized>>);
static_assert(std::same_as<decltype(subrange(a, nullptr, 0)),
    subrange<int*, std::nullptr_t, std::ranges::subrange_kind::sized>>);
libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp
48 ↗(On Diff #349129)

base_

83 ↗(On Diff #349129)

value_ (but actually, I think you should just call this base_ — see? the underscore has a purpose! :))

127–128 ↗(On Diff #349129)

I think there is not. (Also you wrote unsized for unsigned)

164 ↗(On Diff #349129)

s/enl/nel/g
Also s/Foward/Forward/g below, and probably some others; do a typo-checking pass on this file.

libcxx/test/std/ranges/range.utility/range.subrange/general.compile.pass.cpp
24

Formatting nit: class... Args with a space.
(Also, as above, please don't namespace ranges =; you never even use it in this test file!)

ldionne added inline comments.Thu, Jun 3, 12:51 PM
libcxx/include/__ranges/subrange.h
65

Ack.

66

@zoecarver If you haven't been using underscores lately, then it's simply an oversight in the code review. libc++ has been using trailing underscores for members forever, this isn't something new. We could definitely benefit from documenting the coding style we want, however fixing the issue in this review shouldn't be gated on that.

98–100

Arthur's comment is specifically why I requested that change.

zoecarver marked 31 inline comments as done.Fri, Jun 4, 11:08 AM
zoecarver added inline comments.
libcxx/include/__ranges/subrange.h
49–52

I think it's important to make sure this is the same as the standard naming. Luckily the standard changed this for us.

50–51

I've been informed that these are not ADL. Fixed.

66

OK I'll update these. Also, I'll add to my list of things I probably won't ever find time for: configure the clang tidy rule for this.

116–120

@cjdb Ping.

149

I'm just guessing here, but maybe LWG didn't think that empty was sufficiently vexing. As I've already discussed, it makes sense to mark next/prev nodiscard because there are multiple APIs that do different things there, and begin needs to be nodiscard because it's moving the iterator. Empty doesn't have anything "special" (and reason to mark it nodiscard) other than a confusing name.

184

Any reason for this-> over _Base::?

Or would that not-work for some technical reason?

Pretty sure there's a clang bug that doesn't allow us to use inline-requires like this.

libcxx/test/std/ranges/range.utility/range.subrange/ctad.compile.pass.cpp
27

Did something similar, it's a bit better now.

libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp
127–128 ↗(On Diff #349129)

Removed. Thanks.

zoecarver marked 6 inline comments as done.Fri, Jun 4, 11:10 AM
zoecarver added inline comments.
libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp
164 ↗(On Diff #349129)

I'm bad at spotting these, so if you see others let me know and I'll fix them.

zoecarver updated this revision to Diff 349933.Fri, Jun 4, 11:42 AM

Apply review comments.

ldionne accepted this revision.Mon, Jun 7, 1:50 PM

LGTM with my comments.

libcxx/include/__ranges/subrange.h
164

Can we add tests that check that those methods are [[nodiscard]], since those are mandated by the standard?

184

Any reason for this-> over _Base::?

Nope, either way's fine as far as I'm concerned.

(I'd like @cjdb to take a final look at this and give the green light if he's fine with it, since he had a previous requires-changes)

Quuxplusone added inline comments.Tue, Jun 8, 5:29 AM
libcxx/include/__ranges/subrange.h
184

I strongly prefer this->__size_. Don't use weird syntax unless there's a technical reason for it.

cjdb accepted this revision.Tue, Jun 8, 9:48 AM

Thanks for working hard on this!

libcxx/include/__ranges/subrange.h
98–100

I'm with Zoe on this one. We have an underscore-suffix-for-members convention, so it should be clear that __member_ is shorthand for this->__member_, and _Base::__member_ is for the base's member. Whether or not it's an instance member or a static member feels largely irrelevant to me?

116–120

Ah, yes, I missed that, sorry.

144

Yes. Tim's GH link has all the relevant info.

This revision is now accepted and ready to land.Tue, Jun 8, 9:48 AM
cjdb requested changes to this revision.Thu, Jun 10, 8:58 AM

Please update according to the recent changes made by P2325.

This revision now requires changes to proceed.Thu, Jun 10, 8:58 AM

Apply review comments.

Will land when bots are green.

The above update:

  • Split tests into multiple files.
  • Rebased onto D102468 and applied changes from Barry's paper.
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Jun 11, 9:34 AM
This revision was automatically updated to reflect the committed changes.