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

Fri Jun 4 11:08:57 PDT 2021

zoecarver marked 31 inline comments as done.
zoecarver added inline comments.

================
Comment at: libcxx/include/__ranges/subrange.h:49-50
+      typename tuple_element_t<1, remove_const_t<_Tp>>;
+      { get<0>(__t) } -> convertible_to<const tuple_element_t<0, _Tp>&>;
+      { get<1>(__t) } -> convertible_to<const tuple_element_t<1, _Tp>&>;
+    };
----------------
Quuxplusone wrote:
> 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`.
I've been informed that these are not ADL. Fixed. 

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

================
Comment at: libcxx/include/__ranges/subrange.h:148
+
+    [[nodiscard]] constexpr bool empty() const { return __begin == __end; }
+
----------------
ldionne wrote:
> 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.
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. 

================
Comment at: libcxx/include/__ranges/subrange.h:183
+          if constexpr (__store_size)
+            _Base::__size += __to_unsigned_like(-__n);
+          return *this;
----------------
Quuxplusone wrote:
> `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?
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. 

================
Comment at: libcxx/include/__ranges/subrange.h:48-51
+    // If they're both pointers, they must have the same element type.
+    !(is_pointer_v<decay_t<_From>> &&
+      is_pointer_v<decay_t<_To>> &&
+      __not_same_as<remove_pointer_t<decay_t<_From>>, remove_pointer_t<decay_t<_To>>>);
----------------
cjdb wrote:
> Checking that the comment isn't the opposite of what the code wants is a bit convoluted. I suggest making a named concept out of this bit to improve readability.
I think it's important to make sure this is the same as the standard naming. Luckily the standard changed this for us. 

================
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:
> cjdb wrote:
> > I believe you also need an overload for when `__store_size == false`.
> Other than the one on L112?
@cjdb Ping.

================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctad.compile.pass.cpp:26
+static_assert(std::same_as<decltype(subrange(static_cast<int*>(nullptr), static_cast<int*>(nullptr), 0)), subrange<int*, int*, std::ranges::subrange_kind::sized>>);
+static_assert(std::same_as<decltype(subrange(static_cast<int*>(nullptr), nullptr, 0)), subrange<int*, nullptr_t, std::ranges::subrange_kind::sized>>);
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Please add some linebreaks to keep this readable. Same for the other places with these overly long lines. IIRC our current maximum column width is 120.
> 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>>);
> ```
Did something similar, it's a bit better now. 

================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp:127-128
+static_assert( std::is_constructible_v<SizedIntPtrSubrange, int*, int*, size_t>); // 5.
+// TODO: is there any integral type that is not convertible to an unsized type?
+// static_assert(!std::is_constructible_v<SizedSentinelFowardSubrange, SizedSentinelForwardIter, SizedSentinelForwardIter, SizedSentinelForwardIter::difference_type>); // Not unsigned.
+
----------------
Quuxplusone wrote:
> I think there is not. (Also you wrote `unsized` for `unsigned`)
Removed. Thanks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006