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

[Jonathan Wakely via Phabricator via libcxx-commits]

jwakely 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);
----------------
Quuxplusone wrote:
> ldionne wrote:
> > CaseyCarter wrote:
> > > Quuxplusone wrote:
> > > > 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?
> > > > 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.
> > > 
> > > It is unfortunate that the wording requires the difference to be performed when `n == 0`, and the assignment to be performed even when `n == 0` and `last - first == 0`, which implies `first == last` already. MSVC does compute the distance (I think `n == 0` is a weird enough corner not to be worth the cost of checking) but not the assignment when `n == 0`.  I suggest you implement what you think is most performant, and file an LWG issue to allow your implementation and ours.
> > > 
> > > I would prefer to inline it at its one call site, here, rather than generalize it unnecessarily.
> > 
> > After looking at the code generation and seeing that we are indeed paying for the generality, I agree that we should special case it: https://godbolt.org/z/Ez6W9Yfej. I'll still use a lambda just to avoid having a super complicated inlined `if` condition. I tried various implementations and yours was basically the most efficient and correct one. Even using `if-else` instead of a ternary somehow produces slightly worse code.
> > 
> > > [about evaluating `__bound - __i` when `__n == 0`]
> > 
> > I still think it's better not to generate a special check for `__n == 0`, since that should be fairly rare (as Casey mentioned).
> > 
> > > 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?
> > 
> > I looked, and I think it boils down to libstdc++ not protecting against signed integer overflow like what we fixed back in D106735. @jwakely , you might be interested in taking a look, here's a distilled version of libc++ vs libstdc++: https://godbolt.org/z/aYbq1nxhf
> > 
> I continue to see no reason to name `__magnitude_geq`, nor to define `__M` as `const` ( https://quuxplusone.github.io/blog/2022/01/23/dont-const-all-the-things/ ), nor to define `__M` using C++20 if-syntax instead of making it an ordinary declaration on its own line. But whatevs.
> Vice versa, I'd prefer to see lines 154-155 either done as four lines, or done as one ternary. It's basically a (Fortran ;)) three-way arithmetic `IF`: if `a<0` do one thing, else if `a==0` do another, else if `a>0` do a third.
> 
> It occurs to me that (unless I screwed up the math) it can also be done branchless:
> ```
>     auto __M = __bound - __i;
>     if (((__n < 0) | (__n >= __M)) & ((__n > 0) | (__n <= __M))) {
>         __i = std::move(__bound);
>         return __n - __M;
>     }
> 
>     __i += __n;
>     return 0;
> ```
> but this does not seem like better codegen on GCC, and not a clear-cut winner on Clang either, so I'd go with what you've got.
>  I suggest you implement what you think is most performant, and file an LWG issue to allow your implementation and ours.

Yes, please do. I don't care about `n==0` but doing a pointless assignment for `bound - i == 0` is pointless.

>  @jwakely , you might be interested in taking a look

Thanks, I'd missed the reflector thread about it last July.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117240