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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/advance.h:171
       // If |n| >= |bound - i|, equivalent to `ranges::advance(i, bound)`.
       if (const auto __M = __bound - __i; __magnitude_geq(__n, __M)) {
         (*this)(__i, __bound);
----------------
I would prefer to inline it at its one call site, here, rather than generalize it unnecessarily.

Unfortunately https://eel.is/c++draft/range.iter.op.advance#6.1.1 seems to mandate that even when `__n == 0`, we need to evaluate `__bound - __i` and then, if that's also zero, we must `__i = __bound` instead of simply returning immediately with `0`. Is there any blanket-wording loophole that gets us out of that obligation? This code could get much simpler if we were allowed to put `if (__n == 0) return 0;` on line 168.

Also, notice that if `!(bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>)`, then comparing `__n <= __M` is going to be superfluous — we know `__n` can't be negative unless `(bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>)`, so, if `!(bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>)`, it would be really nice not to generate any test for that possibility.

https://godbolt.org/z/jE66PEP4E — I haven't really looked at this code but it sure //looks// like libc++ is doing something suboptimal right now, doesn't it?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp:154
     }
   }
 
----------------
This is obviously better than it was. But I'd love to see some simple regression tests like we were doing on Discord:
```
int a[10];
int *p;
const int *pc;
using ConstPtr = const int*;

// forward with same type
p = a+5; assert(std::ranges::advance(p, 0, a+7) == 0); assert(p == a+5);
p = a+5; assert(std::ranges::advance(p, 1, a+7) == 0); assert(p == a+6);
p = a+5; assert(std::ranges::advance(p, 2, a+7) == 0); assert(p == a+7);
p = a+5; assert(std::ranges::advance(p, 3, a+7) == 1); assert(p == a+7);

// forward with different, non-assignable sentinel
p = a+5; assert(std::ranges::advance(p, 0, ConstPtr(a+7)) == 0); assert(p == a+5);
p = a+5; assert(std::ranges::advance(p, 1, ConstPtr(a+7)) == 0); assert(p == a+6);
p = a+5; assert(std::ranges::advance(p, 2, ConstPtr(a+7)) == 0); assert(p == a+7);
p = a+5; assert(std::ranges::advance(p, 3, ConstPtr(a+7)) == 1); assert(p == a+7);

// forward with different, assignable sentinel
pc = a+5; assert(std::ranges::advance(pc, 0, a+7) == 0); assert(pc == a+5);
pc = a+5; assert(std::ranges::advance(pc, 1, a+7) == 0); assert(pc == a+6);
pc = a+5; assert(std::ranges::advance(pc, 2, a+7) == 0); assert(pc == a+7);
pc = a+5; assert(std::ranges::advance(pc, 3, a+7) == 1); assert(pc == a+7);

// backward with same type
p = a+5; assert(std::ranges::advance(p, 0, a+3) == 0); assert(p == a+5);
p = a+5; assert(std::ranges::advance(p, -1, a+3) == 0); assert(p == a+4);
p = a+5; assert(std::ranges::advance(p, -2, a+3) == 0); assert(p == a+3);
p = a+5; assert(std::ranges::advance(p, -3, a+3) == -1); assert(p == a+3);

// backward with different type is UB so don't test it
```
This covers all the cases, I think. The only other thing that needs testing is whether we assign from Sent to It in all the right places (and not in the wrong places); that'll require a sentinel type with a user-defined `operator=` so we can see that it's being called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117240