[libcxx-commits] [PATCH] D101922: [libcxx][iterator] adds `std::ranges::advance`
Christopher Di Bella via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sat May 8 10:35:30 PDT 2021
cjdb added inline comments.
================
Comment at: libcxx/include/__iterator/primitives.h:10
+
+#ifndef _LIBCPP___ITERATOR_PRIMITIVES_H
+#define _LIBCPP___ITERATOR_PRIMITIVES_H
----------------
ldionne wrote:
> Why is this header named that way? Shouldn't this be `__iterator/advance.h` to be consistent with what we've done so far? We've never named headers after the section of the standard that they roughly represent, and I don't think we should start doing that. Section names can change, they don't necessarily best represent what's in the file, and it goes against what we've started doing consistently.
Same reason we have `__ranges/access.h`. I can split it up, but it feels weird to do two different things here.
================
Comment at: libcxx/include/__iterator/primitives.h:39
+ template <signed_integral _Tp>
+ [[nodiscard]] static constexpr make_unsigned_t<_Tp> __abs(_Tp const __n) noexcept {
+ auto const __unsigned_n = __to_unsigned_like(__n);
----------------
ldionne wrote:
> Drop `[[nodiscard]]` (as discussed offline). Also applies to most other places where you use it.
>
> Also, should this be promoted to a more general utility?
>
> And also, this may be obvious to you, but why don't we simply use `std::abs`? Is it because http://eel.is/c++draft/iterator.primitives#range.iter.op.advance-6.1.1 uses absolute value in math notation (which is well-defined for all inputs), whereas `std::abs` is UB if the absolute value can't be represented in the return type?
> Drop `[[nodiscard]]` (as discussed offline). Also applies to most other places where you use it.
>
> Also, should this be promoted to a more general utility?
>
> And also, this may be obvious to you, but why don't we simply use `std::abs`? Is it because http://eel.is/c++draft/iterator.primitives#range.iter.op.advance-6.1.1 uses absolute value in math notation (which is well-defined for all inputs), whereas `std::abs` is UB if the absolute value can't be represented in the return type?
`std::abs` isn't constexpr, so that's a no-go. I can promote it to a general utility. Which header would you like it in?
================
Comment at: libcxx/include/__iterator/primitives.h:60-70
+ while (__n > 0) {
+ --__n;
+ ++__i;
+ }
+
+ // Otherwise, decrements `i` by `-n`.
+ if constexpr (bidirectional_iterator<_Ip>) {
----------------
ldionne wrote:
> I understand why this works, but I find it's a slightly confusing way to write this. Instead, Maybe `else if constexpr (is_bidirectional_iterator<_Ip>) { handle bidirectional case } else { handle-forward-only-case }`.
>
> It's not as clever, but maybe easier to understand?
I can do that, but it results in code duplication.
```
else if constexpr (bidirectional_iterator<_Ip>) {
while (__n > 0) {
--__n;
++_i;
}
while (__n < 0) {
++__n;
--__i;
}
}
else {
while (__n > 0) {
--__n;
++_i;
}
}
```
I suppose I could put the forward case into an `__advance_forward` and call that twice. WDYT?
```
else if constexpr (bidirectional_iterator<_Ip>) {
__advance_forward(__i, __n);
__advance_backward(__i, __n);
}
else {
__advance_forward(__i, __n);
}
```
================
Comment at: libcxx/include/__iterator/primitives.h:99
+ template <input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
+ [[nodiscard]] constexpr iter_difference_t<_Ip> operator()(_Ip& __i, iter_difference_t<_Ip> __n, _Sp __bound) const {
+ _LIBCPP_ASSERT(__n >= 0 || (bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>),
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > cjdb wrote:
> > > zoecarver wrote:
> > > > cjdb wrote:
> > > > > zoecarver wrote:
> > > > > > This is a value we likely actually do want to discard.
> > > > > Please elaborate?
> > > > If we did the thing I wish we did in C++ and functions were nodiscard by default, there would be a `[[discardable_result]]` attribute instead.
> > > >
> > > > If that were the case, this function would be one of the ones that's marked with `[[discardable_result]]`, because there are valid reasons to use this function and discard the result. That's probably the most common use, in fact.
> > > >
> > > > In other words, this function is one of the ones where we don't actually need the nodiscard attribute.
> > > > In other words, this function is one of the ones where we don't actually need the nodiscard attribute.
> > >
> > > What I meant was "please elaborate on why you feel this function's result should be considered discardable".
> > `std::ranges::advance(i, s); // advance i to s`
> > This is the //normal and expected// use of the function: for its side effect. It's just like `std::advance`. When we get to `std::ranges::next`, you'll have a stronger case for `_LIBCPP_NODISCARD_EXT`.
> >
> > Also, again, please make sure to use `_LIBCPP_NODISCARD_EXT` for things that are non-nodiscard according to the Standard Itself. We can mark them, but we need to be clear that it's a libc++ extension. libc++ uses raw `[[nodiscard]]` only in places that the Standard Itself uses `[[nodiscard]]` — a rare occurrence.
> Because it has side effects: advancing the iterator. It's possible, and even likely, that users would only care about the side effects. One example of this is in the implementation of `ranges::next` another is in my `drop_view` patch.
> Also, again, please make sure to use `_LIBCPP_NODISCARD_EXT` for things that are non-nodiscard according to the Standard Itself. We can mark them, but we need to be clear that it's a libc++ extension.
Also, again, why do we need to be clear that it's an extension? What does `_LIBCPP_NODISCARD_EXT` offer us over using `[[nodiscard]]` and the user disabling the extension?
> libc++ uses raw `[[nodiscard]]` only in places that the Standard Itself uses `[[nodiscard]]` — a rare occurrence.
That's because no one has gone to the effort of adding `[[nodiscard]]` to everything that should have it. I tried to change this for ranges in C++20, but LEWG felt the effort at the NB comment stage was to much when we had scores (if not hundreds) of other comments to look into and forward to LWG, so they asked me to defer to C++23. That's a paper I should revisit.
> Because it has side effects: advancing the iterator. It's possible, and even likely, that users would only care about the side effects.
I still feel that this is information that the user shouldn't be implicitly discarding, but it looks like I've got @ldionne, @zoecarver, and @Quuxplusone against, so I'm going to remove it.
================
Comment at: libcxx/include/iterator:550
+template <class _InputIter, class _Distance,
+ class = typename enable_if<is_integral<decltype(_VSTD::__convert_to_integral(declval<_Distance>()))>::value>::type>
inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
----------------
ldionne wrote:
> What's the rationale for this change? I remember playing around the same area here: https://reviews.llvm.org/D81425, it might be useful to read that again for context. I don't have enough time to form an opinion again right now unfortunately.
>
> If we decide to keep it, we should add a test. IIUC that would be considered a libc++ extension at this point based on glancing at https://reviews.llvm.org/D81425 again quickly.
Making an unqualified call to `advance(i, s)` will yield `no matching function for call to '__convert_to_integral'` otherwise. That's definitely an implementation detail leaking out of the interface, and suppresses what I think is the more natural diagnostic: `no matching function for call to 'advance'`.
> If we decide to keep it, we should add a test. IIUC that would be considered a libc++ extension at this point based on glancing at https://reviews.llvm.org/D81425 again quickly.
I'll have a read and report my opinions here. I'm actually surprised that `std::advance` doesn't constrain this function on `Distance` being convertible to the iterator's distance type! That's probably an LWG defect there.
================
Comment at: libcxx/test/support/test_iterators.h:783
+ ++distance_traversed_;
+ ++displacement_;
+ return *this;
----------------
zoecarver wrote:
> Maybe we should have a `operations_count` too. I feel like these should be `+= n` but that's not very helpful for testing complexity.
Hmm that's a better name. I'm not really tracking the distance: I'm tracking the number of forward and backward operations. WDYT about
* `s/distance_traversed/operation_count/g`
* `s/displacement/operation_displacement/g`
With the exception of input iterators, tracking distance isn't useful since we'll very soon have `std::ranges::distance`. For the input iterator case, `operation_count` is synonymous.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101922/new/
https://reviews.llvm.org/D101922
More information about the libcxx-commits
mailing list