[libcxx-commits] [PATCH] D117240: [libc++] Fix bug in ranges::advance and refactor the tests

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__iterator/advance.h:80-91
+  static constexpr bool __magnitude_geq(_Tp __a, _Tp __b) {
+    if (__a < 0)
+      if (__b < 0)
+        return __a <= __b;
+      else
+        return __a + __b <= 0;
+    else
----------------
ldionne wrote:
> I'm open to bikeshedding this implementation.
> 
> AFAICT, this implementation is a general-purpose magnitude->= implementation that doesn't suffer from overflow (I think).
> 
> We technically don't need something as general, but my preference would be to keep it general unless we have a noticeably more performant implementation, because keeping it general makes it easier to understand.
If you don't want to do the early returns at least put braces around the outer if and else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117240