[libcxx-commits] [PATCH] D101922: [libcxx][iterator] adds `std::ranges::advance`

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a comment.

LGTM modulo some small nits.



================
Comment at: libcxx/include/__function_like.h:10
+
+#ifndef _LIBCPP___ITERATOR_FUNCTIONISH_H
+#define _LIBCPP___ITERATOR_FUNCTIONISH_H
----------------
Please also rename the include guard.


================
Comment at: libcxx/include/__iterator/primitives.h:41
+    auto const __unsigned_n = static_cast<make_unsigned_t<_Tp> >(__n);
+    return __n < 0 ? -__unsigned_n : __unsigned_n;
+  }
----------------
I'm not too fond of negating unsigned values. How do you feel about method/approach used in `<charconv>`?
Here the code I'm referring to, mainly `__complement`, but some added context.
```
template <typename _Tp>
inline _LIBCPP_INLINE_VISIBILITY _Tp
__complement(_Tp __x)
{
    static_assert(is_unsigned<_Tp>::value, "cast to unsigned first");
    return _Tp(~__x + 1);
}

template <typename _Tp>
inline _LIBCPP_INLINE_VISIBILITY typename make_unsigned<_Tp>::type
__to_unsigned(_Tp __x)
{
    return static_cast<typename make_unsigned<_Tp>::type>(__x);
}

template <typename _Tp>
_LIBCPP_AVAILABILITY_TO_CHARS
inline _LIBCPP_INLINE_VISIBILITY to_chars_result
__to_chars_itoa(char* __first, char* __last, _Tp __value, true_type)
{
    auto __x = __to_unsigned(__value);
    if (__value < 0 && __first != __last)
    {
        *__first++ = '-';
        __x = __complement(__x);
    }

    return __to_chars_itoa(__first, __last, __x, false_type());
}
```


================
Comment at: libcxx/include/__iterator/primitives.h:39
+  [[nodiscard]] static constexpr auto __abs(_Tp const __n) noexcept {
+    _LIBCPP_ASSERT(__n != numeric_limits<_Tp>::min(), "`-numeric_limits<I>::min()` is undefined");
+    return __n < 0 ? -__n : __n;
----------------
cjdb wrote:
> Mordante wrote:
> > Can't we return the unsigned version of `_Tp`? Then  we can return the largest negative value.
> I suppose we could. I don't like treating unsigned ints as "non-negative whole numbers", but I don't think we've got a choice here.
In general I also dislike this way to treat unsigned values. But I think it's better than disallowing one value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922