[libcxx-commits] [PATCH] D102006: [libcxx][ranges] Add range.subrange.
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jun 2 20:38:43 PDT 2021
Quuxplusone 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>&>;
+ };
----------------
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`.
================
Comment at: libcxx/include/__ranges/subrange.h:65
+ static constexpr bool __store_size = false;
+ _Iter __begin = _Iter();
+ _Sent __end = _Sent();
----------------
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."
================
Comment at: libcxx/include/__ranges/subrange.h:97-99
+ using _Base::__store_size;
+ using _Base::__begin;
+ using _Base::__end;
----------------
zoecarver wrote:
> 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.
FWIW, I agree with @ldionne: `using`-declarations (and their effect on name lookup) are significantly more confusing for the reader/maintainer than the plain old `this->__member_` expression syntax we're all familiar with.
================
Comment at: libcxx/include/__ranges/subrange.h:142
+
+ [[nodiscard]] constexpr _Iter begin() requires (!copyable<_Iter>) {
+ return _VSTD::move(__begin);
----------------
zoecarver wrote:
> 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.
Tangential, but yeah: it wouldn't make sense to require the caller to write `std::move(myRange).begin()`, because then how would you ever get at the end-sentinel? You couldn't write again `std::move(myRange).end()`, because that would trigger all your static-analyzer's "double-moved-from" checks.
================
Comment at: libcxx/include/__ranges/subrange.h:183
+ if constexpr (__store_size)
+ _Base::__size += __to_unsigned_like(-__n);
+ return *this;
----------------
`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?
================
Comment at: libcxx/include/__ranges/subrange.h:208-209
+ template<borrowed_range _Range>
+ subrange(_Range&&, make_unsigned_t<range_difference_t<_Range>>) ->
+ subrange<iterator_t<_Range>, sentinel_t<_Range>, subrange_kind::sized>;
+
----------------
Formatting nit: the `->` belongs on the second line. Lines 199-200 above do it right AFAIC.
================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/access.pass.cpp:58
+
+using std::ptrdiff_t;
+struct SizedSentinelForwardIter {
----------------
Definitely just write out `std::ptrdiff_t` in the like two places it appears.
================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/access.pass.cpp:73
+
+ constexpr friend bool operator==(const self& lhs, const self& rhs) { return lhs.base == rhs.base; }
+
----------------
Formatting nit: `friend constexpr`
================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctad.compile.pass.cpp:21
+
+using std::ranges::subrange;
+
----------------
ldionne wrote:
> `namespace ranges = std::ranges` or something like that, but we frown upon importing names.
(moving my comment from the previous file after seeing Louis's)
Looking through this test file, I don't think skipping the `std::` on lines 89–163 is really buying you anything. Strongly consider just writing out `std::ranges::whatever` everywhere it needs to be. (The benefit is greppability.)
================
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>>);
+
----------------
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>>);
```
================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp:48
+
+ int *base = nullptr;
+
----------------
`base_`
================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp:83
+
+ int *value = nullptr;
+
----------------
`value_` (but actually, I think you should just call this `base_` — see? the underscore has a purpose! :))
================
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.
+
----------------
I think there is not. (Also you wrote `unsized` for `unsigned`)
================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/ctor.pass.cpp:164
+
+using DifferentSentienlSubrange = ranges::subrange<ForwardIter,
+ ForwardBorrowedRangeDifferentSentienl::sentinel,
----------------
`s/enl/nel/g`
Also `s/Foward/Forward/g` below, and probably some others; do a typo-checking pass on this file.
================
Comment at: libcxx/test/std/ranges/range.utility/range.subrange/general.compile.pass.cpp:23
+
+template<std::ranges::subrange_kind K, class...Args>
+concept ValidSubrangeKind = requires { typename std::ranges::subrange<Args..., K>; };
----------------
Formatting nit: `class... Args` with a space.
(Also, as above, please don't `namespace ranges =`; you never even use it in this test file!)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102006/new/
https://reviews.llvm.org/D102006
More information about the libcxx-commits
mailing list