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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked an inline comment as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/subrange.h:134
+    constexpr subrange(_Range&& __range)
+      requires sized_range<_Range>
+      : subrange(__range, ranges::size(__range)) { }
----------------
zoecarver wrote:
> cjdb wrote:
> > You'll probably need to guard on `!__store_size && sized_range<R>`, since it's possible for both to be true, thus creating an ambiguity. Please add a test for this very case.
> Hmm, I think you're right. So that would be the case where `!sized_sentinel_for` but it had a `size` member or something?
For posterity: this is actually the opposite of what we need to guard on, we need to guard on `__store_size && sized_range<R>`.

Because for this overload:
```
constexpr subrange(R&& r) requires (!StoreSize || sized_­range<R>);
```
We do 
 - `subrange(r, size)` when `StoreSize`
 - `subrange(begin, end)` when `!StoreSize`

However, it was just easier to write it as two overloads where we do
 - `subrange(begin, end)` when `!StoreSize`
 - `subrange(r, size)` when `sized_range`

The problem is that didn't catch the case (as I said above) where both `sized_range` and `!StoreSize` are true, in which case it would be ambiguous. I fixed it and added `DifferentSentienlWithSizeMember` as a regression test. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102006