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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I like the `Implements [range.iter.op.advance]` part in the description, makes finding the documentation a lot easier!
Is the synopsis up to date?



================
Comment at: libcxx/include/__iterator/functionish.h:31
+//
+// Since these are still standard library functions, we use `__functionish` to eliminate most of
+// the properties that function objects get by default (e.g. semiregularity, addressability), to
----------------
Can we use a clearer name, for example `__function_like`, `ish` sounds rather fishy to me. Or maybe something with ADL in the name, but I've no nice name for that to offer.


================
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;
----------------
Can't we return the unsigned version of `_Tp`? Then  we can return the largest negative value.


================
Comment at: libcxx/include/__iterator/primitives.h:56
+      return;
+    } else {
+      // Otherwise, if `n` is non-negative, increments `i` by `n`.
----------------
Can you remove the `else`?


================
Comment at: libcxx/include/__iterator/primitives.h:112
+      return 0;
+    } else {
+      // Otherwise, if `n` is non-negative, while `bool(i != bound)` is true, increments `i` but at
----------------
Can you remove the `else`?


================
Comment at: libcxx/include/__iterator/primitives.h:129
+    }
+  }
+};
----------------
If we violate the preconditions we leave the function without returning a value, resulting in UB. Can you add `_LIBCPP_UNREACHABLE()` instead.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp:15
+
+#define _LIBCPP_DEBUG_LEVEL 2
+#include <iterator>
----------------
Please use
```
// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DEBUG=1                                  
// UNSUPPORTED: libcxx-no-debug-mode                                            
```
instead.
I wonder whether we still need to use this at all now we have a CI build with debug mode enabled. I will probably remove them from my format patches.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922