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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/subrange.h:183
+          if constexpr (__store_size)
+            _Base::__size += __to_unsigned_like(-__n);
+          return *this;
----------------
ldionne wrote:
> 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.
I strongly prefer `this->__size_`. Don't use weird syntax unless there's a technical reason for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006