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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked an inline comment as done.
cjdb added inline comments.


================
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
----------------
Mordante wrote:
> 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.
`__function_like` wfm. There's more going on here than just ADL inhibition, so we'd need a very good replacement.


================
Comment at: libcxx/include/__iterator/primitives.h:56
+      return;
+    } else {
+      // Otherwise, if `n` is non-negative, increments `i` by `n`.
----------------
Mordante wrote:
> Can you remove the `else`?
This is an `if constexpr` else, which prunes the false case from the codegen when the true path is generated.


================
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
----------------
Mordante wrote:
> Can you remove the `else`?
See above.


================
Comment at: libcxx/include/__iterator/primitives.h:129
+    }
+  }
+};
----------------
Mordante wrote:
> If we violate the preconditions we leave the function without returning a value, resulting in UB. Can you add `_LIBCPP_UNREACHABLE()` instead.
Oooooh thanks for this! I was sad that `_LIBCPP_ASSERT` was so... relaxed.


================
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>
----------------
Mordante wrote:
> 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.
> 
That's just something I forgot to delete. I was trying to get my `_LIBCPP_ASSERT` to fire since I was sure a precondition was bad and forgot to delete it. `_LIBCPP_UNREACHABLE` will render the debug stuff unnecessary anyway, I hope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922