[libcxx-commits] [PATCH] D102006: [libcxx][ranges] Add range.subrange.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb accepted this revision.
cjdb added a comment.
This revision is now accepted and ready to land.

Thanks for working hard on this!



================
Comment at: libcxx/include/__ranges/subrange.h:97-99
+    using _Base::__store_size;
+    using _Base::__begin;
+    using _Base::__end;
----------------
ldionne wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > ldionne wrote:
> > > > 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?
> > > 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. 
> > 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.
> Arthur's comment is specifically why I requested that change.
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?


================
Comment at: libcxx/include/__ranges/subrange.h:115-119
+
+    constexpr subrange(__convertible_to_non_slicing<_Iter> auto __iter, _Sent __sent,
+                       make_unsigned_t<iter_difference_t<_Iter>> __n)
+      requires (_Kind == subrange_kind::sized)
+      : _Base(_VSTD::move(__iter), __sent, __n) { }
----------------
zoecarver wrote:
> zoecarver wrote:
> > cjdb wrote:
> > > I believe you also need an overload for when `__store_size == false`.
> > Other than the one on L112?
> @cjdb Ping.
Ah, yes, I missed that, sorry.


================
Comment at: libcxx/include/__ranges/subrange.h:143
+      requires (_Kind == subrange_kind::sized)
+      : subrange(ranges::begin(__range), ranges::end(__range), __n) { }
+
----------------
zoecarver wrote:
> tcanens wrote:
> > cjdb wrote:
> > > Standard uses braces for this one.
> > > Standard uses braces for this one.
> > 
> > For ranges wording, wherever braces have behavior different from parens, the use of braces is almost certainly a defect. P2367R0 is expected to fix all such occurrences. https://github.com/cplusplus/draft/issues/4593 deals with the rest.
> Is there an observable difference/do we care? If there's no difference I try to use parens, because that shows there's no difference (if I use braces, a reader might ask "is there a reason we have to use braces here?"). 
Yes. Tim's GH link has all the relevant info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006