[libc++] [ranges] Add namespace __cpo to ranges::{advance,next,prev}.

[• Quuxplusone]

Summary

The reason for those nested namespaces is explained in D115315:

AIUI, this keeps the CPO's own type from ADL'ing into the std::ranges namespace; e.g. foobar(std::ranges::uninitialized_default_construct) should not consider std::ranges::foobar a candidate, even if std::ranges::foobar is not a CPO itself. Also, of course, consistency (Chesterton's Fence, the economist's hundred-dollar bill): if it were safe to omit the namespace, we'd certainly want to do it everywhere, not just here.

This makes these three niebloids more consistent with the other Ranges niebloids we've already implemented, such as the ranges::begin group and the ranges::uninitialized_default_construct group.

FWIW, we still have three different indentation-and-comment styles among these three groups.

[ldionne]

Thanks for making this more consistent with the other niebloids.

[var-const]

The main intention of the patch very much LGTM, but some formatting nitpicks. Please let me know if I'm missing something.

libcxx/include/__iterator/advance.h

74

Formatting nit: I think namespaces aren't supposed to introduce a new level of indentation.

75

It looks like it's using tabs for indentation now, is this intentional?

192

Same nit here: I think this variable declaration shouldn't be indented.

[• Quuxplusone]

libcxx/include/__iterator/advance.h

74

In general yeah, but this is consistent with most other niebloids/CPOs in libc++ today.
git grep -h struct.__fn libcxx/include/

75

On my browser screen I see little green >> chevrons, indicating that this all bumped over 2 spaces (intentionally); but that's not related to the choice of tabs/spaces.
In fact, CI will be unhappy with tabs, so if CI is green you can assume there are no tabs. Anyway, I'll double-check just to be sure, but I'm 99.99% sure that my editor is too dumb to insert tabs unintentionally. :)

192

Again this is for consistency; and here (unlike above) we're almost 100% consistent with the indentation.
git grep -h 'inline constexpr auto.*__fn' libcxx/include/

[var-const]

libcxx/include/__iterator/advance.h

74

I don't see a reason for niebloids to diverge from the style guide, and I don't think we should try to be consistent with things that are incorrectly (IMO) formatted. That would largely defeat the purpose of having a style guide if, once an inconsistent piece of code gets into the code base, it effectively takes precedence over the style guide.

75

Yes, I took >> to mean tabs -- if this isn't the case, please disregard.

[ldionne]

libcxx/include/__iterator/advance.h

74

I had missed it, but it does seem more consistent *not* to indent the content of long namespaces since that's what we tend to do elsewhere. Also. it means touching fewer lines, which I guess is good.

[• Quuxplusone]

libcxx/include/__iterator/advance.h

74

So, leave inline constexpr auto niebloid = indented, but de-dent struct __fn throughout?
If that's the request, I'll do the de-dent in a preliminary commit, and then follow it up with this one (minus the indenting here).

[• Quuxplusone]

Remove namespace indentation after discussion offline with @var-const. I think this should be uncontroversial now.

As mentioned (now) in the commit message: The ranges::begin group still uses indentation in their namespaces (e.g. all of namespace ranges::__begin is indented 2 spaces). I thought about "fixing" that in this PR too, but decided I don't want to open that can of worms yet. Let's get this landed first.

[• Quuxplusone]

libcxx/include/__iterator/prev.h

42–43

Oops, meant to add a blank line after line 43 here. Will fix.

[• Quuxplusone]

@var-const: is this now good to go, as far as you're concerned?

[var-const]

In D116569#3225533, @Quuxplusone wrote:

@var-const: is this now good to go, as far as you're concerned?

I've been OOO today, but I'll take a look tomorrow. Thanks for doing the change!

[var-const]

LGTM.

libcxx/include/__iterator/advance.h

void advance(_InputIter& __i, _Distance __orig_n) {
   _Difference __n = static_cast<_Difference>(_VSTD::__convert_to_integral(__orig_n));
   _LIBCPP_ASSERT(__n >= 0 || __is_cpp17_bidirectional_iterator<_InputIter>::value,
                  "Attempt to advance(it, n) with negative n on a non-bidirectional iterator");
   _VSTD::__advance(__i, __n, typename iterator_traits<_InputIter>::iterator_category());
 }
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
-namespace ranges {
 // [range.iter.op.advance]
-// TODO(varconst): rename `__advance_fn` to `__fn`.
-struct __advance_fn final : private __function_like {
+
+namespace ranges {
+namespace __advance {
+  struct __fn final : private __function_like {
   private:
     template <class _Tp>
     _LIBCPP_HIDE_FROM_ABI
     static constexpr _Tp __magnitude_geq(_Tp __a, _Tp __b) noexcept {
       return __a < 0 ? (__a <= __b) : (__a >= __b);
     }
 
     template <class _Ip>
     _LIBCPP_HIDE_FROM_ABI
     static constexpr void __advance_forward(_Ip& __i, iter_difference_t<_Ip> __n) {
       while (__n > 0) {
         --__n;
         ++__i;
       }
     }
 
     template <class _Ip>
     _LIBCPP_HIDE_FROM_ABI
     static constexpr void __advance_backward(_Ip& __i, iter_difference_t<_Ip> __n) {
       while (__n < 0) {
         ++__n;
         --__i;
       }
     }
 
   public:
-  constexpr explicit __advance_fn(__tag __x) noexcept : __function_like(__x) {}
+     constexpr explicit __fn(__tag __x) noexcept : __function_like(__x) {}
 
     // Preconditions: If `I` does not model `bidirectional_iterator`, `n` is not negative.
     template <input_or_output_iterator _Ip>
     _LIBCPP_HIDE_FROM_ABI
     constexpr void operator()(_Ip& __i, iter_difference_t<_Ip> __n) const {
       _LIBCPP_ASSERT(__n >= 0 || bidirectional_iterator<_Ip>,
                      "If `n < 0`, then `bidirectional_iterator<I>` must be true.");
 
       // If `I` models `random_access_iterator`, equivalent to `i += n`.
       if constexpr (random_access_iterator<_Ip>) {
         __i += __n;
         return;
       } else if constexpr (bidirectional_iterator<_Ip>) {
         // Otherwise, if `n` is non-negative, increments `i` by `n`.
         __advance_forward(__i, __n);
         // Otherwise, decrements `i` by `-n`.
         __advance_backward(__i, __n);
         return;
       } else {
         // Otherwise, if `n` is non-negative, increments `i` by `n`.
         __advance_forward(__i, __n);
         return;
       }
     }
 
     // Preconditions: Either `assignable_from<I&, S> || sized_sentinel_for<S, I>` is modeled, or [i, bound) denotes a range.
     template <input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
     _LIBCPP_HIDE_FROM_ABI
     constexpr void operator()(_Ip& __i, _Sp __bound) const {
       // If `I` and `S` model `assignable_from<I&, S>`, equivalent to `i = std::move(bound)`.
       if constexpr (assignable_from<_Ip&, _Sp>) {
         __i = _VSTD::move(__bound);
       }
       // Otherwise, if `S` and `I` model `sized_sentinel_for<S, I>`, equivalent to `ranges::advance(i, bound - i)`.
       else if constexpr (sized_sentinel_for<_Sp, _Ip>) {
         (*this)(__i, __bound - __i);
       }
       // Otherwise, while `bool(i != bound)` is true, increments `i`.
       else {
         while (__i != __bound) {
           ++__i;
         }
       }
     }
 
     // Preconditions:
     //   * If `n > 0`, [i, bound) denotes a range.
     //   * If `n == 0`, [i, bound) or [bound, i) denotes a range.
     //   * If `n < 0`, [bound, i) denotes a range, `I` models `bidirectional_iterator`, and `I` and `S` model `same_as<I, S>`.
     // Returns: `n - M`, where `M` is the difference between the the ending and starting position.
     template <input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
     _LIBCPP_HIDE_FROM_ABI
     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>),
                      "If `n < 0`, then `bidirectional_iterator<I> && same_as<I, S>` must be true.");
       // If `S` and `I` model `sized_sentinel_for<S, I>`:
       if constexpr (sized_sentinel_for<_Sp, _Ip>) {
         // If |n| >= |bound - i|, equivalent to `ranges::advance(i, bound)`.
         if (const auto __M = __bound - __i; __magnitude_geq(__n, __M)) {
           (*this)(__i, __bound);
           return __n - __M;
         }
 
         // Otherwise, equivalent to `ranges::advance(i, n)`.
         (*this)(__i, __n);
         return 0;
       } else {
         // Otherwise, if `n` is non-negative, while `bool(i != bound)` is true, increments `i` but at
         // most `n` times.
         while (__i != __bound && __n > 0) {
           ++__i;
           --__n;
         }
 
         // Otherwise, while `bool(i != bound)` is true, decrements `i` but at most `-n` times.
         if constexpr (bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>) {
           while (__i != __bound && __n < 0) {
             --__i;
             ++__n;
           }
         }
         return __n;
       }
 
       _LIBCPP_UNREACHABLE();
     }
   };
+}
 
-inline constexpr auto advance = __advance_fn(__function_like::__tag());
+inline namespace __cpo {
+  inline constexpr auto advance = __advance::__fn(__function_like::__tag());
+} // namespace __cpo
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ITERATOR_ADVANCE_H

libcxx/include/__iterator/next.h

_LIBCPP_ASSERT(__n >= 0 || __is_cpp17_bidirectional_iterator<_InputIter>::value,
                  "Attempt to next(it, n) with negative n on a non-bidirectional iterator");
 
   _VSTD::advance(__x, __n);
   return __x;
 }
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
+// [range.iter.op.next]
+
 namespace ranges {
-// TODO(varconst): rename `__next_fn` to `__fn`.
-struct __next_fn final : private __function_like {
+namespace __next {
+  struct __fn final : private __function_like {
     _LIBCPP_HIDE_FROM_ABI
-  constexpr explicit __next_fn(__tag __x) noexcept : __function_like(__x) {}
+    constexpr explicit __fn(__tag __x) noexcept : __function_like(__x) {}
 
     template <input_or_output_iterator _Ip>
     _LIBCPP_HIDE_FROM_ABI
     constexpr _Ip operator()(_Ip __x) const {
       ++__x;
       return __x;
     }
 
     template <input_or_output_iterator _Ip>
     _LIBCPP_HIDE_FROM_ABI
     constexpr _Ip operator()(_Ip __x, iter_difference_t<_Ip> __n) const {
       ranges::advance(__x, __n);
       return __x;
     }
 
     template <input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
     _LIBCPP_HIDE_FROM_ABI
     constexpr _Ip operator()(_Ip __x, _Sp __bound) const {
       ranges::advance(__x, __bound);
       return __x;
     }
 
     template <input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
     _LIBCPP_HIDE_FROM_ABI
     constexpr _Ip operator()(_Ip __x, iter_difference_t<_Ip> __n, _Sp __bound) const {
       ranges::advance(__x, __n, __bound);
       return __x;
     }
   };
+}
 
-inline constexpr auto next = __next_fn(__function_like::__tag());
+inline namespace __cpo {
+  inline constexpr auto next = __next::__fn(__function_like::__tag());
+} // namespace __cpo
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ITERATOR_NEXT_H

libcxx/include/__iterator/prev.h

inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
   _LIBCPP_ASSERT(__n <= 0 || __is_cpp17_bidirectional_iterator<_InputIter>::value,
                  "Attempt to prev(it, n) with a positive n on a non-bidirectional iterator");
   _VSTD::advance(__x, -__n);
   return __x;
 }
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
+// [range.iter.op.prev]
+
 namespace ranges {
-// TODO(varconst): rename `__prev_fn` to `__fn`.
-struct __prev_fn final : private __function_like {
+namespace __prev {
+  struct __fn final : private __function_like {
     _LIBCPP_HIDE_FROM_ABI
-  constexpr explicit __prev_fn(__tag __x) noexcept : __function_like(__x) {}
+    constexpr explicit __fn(__tag __x) noexcept : __function_like(__x) {}
 
     template <bidirectional_iterator _Ip>
     _LIBCPP_HIDE_FROM_ABI
     constexpr _Ip operator()(_Ip __x) const {
       --__x;
       return __x;
     }
 
     template <bidirectional_iterator _Ip>
     _LIBCPP_HIDE_FROM_ABI
     constexpr _Ip operator()(_Ip __x, iter_difference_t<_Ip> __n) const {
       ranges::advance(__x, -__n);
       return __x;
     }
 
     template <bidirectional_iterator _Ip>
     _LIBCPP_HIDE_FROM_ABI
     constexpr _Ip operator()(_Ip __x, iter_difference_t<_Ip> __n, _Ip __bound) const {
       ranges::advance(__x, -__n, __bound);
       return __x;
     }
   };
+}
 
-inline constexpr auto prev = __prev_fn(__function_like::__tag());
+inline namespace __cpo {
+  inline constexpr auto prev = __prev::__fn(__function_like::__tag());
+} // namespace __cpo
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ITERATOR_PREV_H