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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM with my comments.



================
Comment at: libcxx/include/__ranges/subrange.h:163
+
+    [[nodiscard]] constexpr subrange next(iter_difference_t<_Iter> __n = 1) && {
+      advance(__n);
----------------
Can we add tests that check that those methods are `[[nodiscard]]`, since those are mandated by the standard?


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

Nope, either way's fine as far as I'm concerned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006