This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Simplify std::ranges::subrange
ClosedPublic

Authored by ldionne on Sep 23 2021, 3:12 PM.

Details

Reviewers
Quuxplusone
ldionne
Group Reviewers
Restricted Project
Commits
rG86df5a2fa832: [libc++] Simplify std::ranges::subrange
Summary

Instead of using a base class to store the members and the optional
size, use [[no_unique_address]] to achieve the same thing without
needing a base class.

Also, as a fly-by:

  • Change subrange from struct to class (per the standard)
  • Improve the diagnostic for when one doesn't provide a size to the ctor of a sized subrange
  • Replace this->member by just member since it's not in a dependent base anymore

This change would be an ABI break due to [[no_unique_address]], but we
haven't shipped ranges anywhere yet, so this shouldn't affect anyone.

Diff Detail

Event Timeline

ldionne requested review of this revision.Sep 23 2021, 3:12 PM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2021, 3:12 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Sep 24 2021, 9:30 AM
Quuxplusone added a subscriber: Quuxplusone.

LbasicallyGTM (and a nice improvement! down with base classes!) mod comments. My trivially-default-constructible comment is significant.

libcxx/include/__ranges/subrange.h
68

Commit message: This cannot be both an ABI break and "[NFCI]". Pick one or the other. ;)

81

This type is not trivially default-constructible. I think you should give it a default ctor _Empty() = default; as well, and then

[[no_unique_address]] _Size __size_ = _Size();

and then your non-sized subrange type will be trivially default-constructible whenever the iterator+sentinel types are. And I think there should be a libcxx/ test for that, because it seems both useful and fragile.

101

If you're moving __iter, you might as well move __sent as well.

169

_VSTD::__to_unsigned_like? Yes; cf. line 210.
Notice that __end_ - __begin_ is sentinel-minus-iterator; I don't know if that's required to be literally iter_difference_t<_Iter>, but I also don't know what else it could be.

This revision now requires changes to proceed.Sep 24 2021, 9:30 AM
ldionne updated this revision to Diff 375400.Sep 27 2021, 2:06 PM
ldionne marked 3 inline comments as done.
ldionne retitled this revision from [libc++][NFCI] Simplify std::ranges::subrange to [libc++] Simplify std::ranges::subrange.
ldionne edited the summary of this revision. (Show Details)

Address review comments

ldionne added inline comments.Sep 27 2021, 2:12 PM
libcxx/include/__ranges/subrange.h
81

I was going to say "awesome, let's do it and add a test for it". Then I tried, and noticed that std::ranges::subrange<int*> still wasn't trivially default constructible, since we have default member initializers per the spec (http://eel.is/c++draft/range.subrange.general).

So I'm not sure we can actually make subrange trivially default constructible, unless I am missing something.

libcxx/include/__ranges/subrange.h
81

Well, subrange<int*> won't be trivially default constructible, but I suppose if you had some iterator type It such that value-initializing It were trivial, then it could be... hmm. Does having a NSDMI actually prevent the class from being trivially default constructible?!
https://godbolt.org/z/1YEWvj71f
Gross. Okay, well, I still like my _Empty() = default idea if it lets you get rid of the icky _Empty(auto) ctor template, but otherwise I guess I have no strong preference either way. I agree it's not going to help with triviality.

ldionne accepted this revision.Sep 28 2021, 2:33 PM
ldionne marked an inline comment as done.
ldionne added inline comments.
libcxx/include/__ranges/subrange.h
81

Does having a NSDMI actually prevent the class from being trivially default constructible?!

Yeah, I was surprised by that too.

Okay, well, I still like my _Empty() = default idea if it lets you get rid of the icky _Empty(auto) ctor template, but otherwise I guess I have no strong preference either way. I agree it's not going to help with triviality.

It doesn't because we have to construct __size_(__n) below.

This revision is now accepted and ready to land.Sep 28 2021, 2:33 PM
This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.