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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


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


================
Comment at: libcxx/include/__ranges/subrange.h:65
+    static constexpr bool __store_size = false;
+    _Iter __begin = _Iter();
+    _Sent __end = _Sent();
----------------
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. 


================
Comment at: libcxx/include/__ranges/subrange.h:80
+
+    constexpr __subrange_base() = default;
+    constexpr __subrange_base(_Iter __iter, _Sent __sent, decltype(__size) __size)
----------------
Nit: no constexpr. 


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


================
Comment at: libcxx/include/__ranges/subrange.h:142
+
+    [[nodiscard]] constexpr _Iter begin() requires (!copyable<_Iter>) {
+      return _VSTD::move(__begin);
----------------
ldionne wrote:
> I'm surprised they didn't make this `_Iter begin() &&` instead to make sure we can only call this on a xvalue.
This is odd and does not seem to be inline with most views. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006