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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/subrange.h:64
+  struct __subrange_base {
+    static constexpr bool __store_size = false;
+    _Iter __begin = _Iter();
----------------
zoecarver wrote:
> ldionne wrote:
> > 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.
> 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). 
Ack.


================
Comment at: libcxx/include/__ranges/subrange.h:65
+    static constexpr bool __store_size = false;
+    _Iter __begin = _Iter();
+    _Sent __end = _Sent();
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > `__begin_`? (same for other member variables)
> > 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. 
> FWIW, I feel strongly: libc++ should follow libc++ style, which is "data members get underscore-suffixed."
@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.


================
Comment at: libcxx/include/__ranges/subrange.h:97-99
+    using _Base::__store_size;
+    using _Base::__begin;
+    using _Base::__end;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006