[libcxx-commits] [PATCH] D107396: [libcxx][ranges] Add `ranges::iota_view`.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/iota_view.h:257-260
+          if (__y.__value_ > __x.__value_) {
+            return difference_type(-difference_type(__y.__value_ - __x.__value_));
+          }
+          return difference_type(__x.__value_ - __y.__value_);
----------------
zoecarver wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > cjdb wrote:
> > > > 
> > > The conditional expression here does not seem easier to read. I could see an argument for it to be the same difficulty, but to me it is harder to read than the if statement. I don't see a reason to change this. 
> > We're at a bit of an impasse then, because I find the if-statement approach to be the less-readable one.
> See discord discussion. 
IIUC, you're discussing the cascade that currently looks like this:
```
        if constexpr (__integer_like<_Start>) {
          if constexpr (__signed_integer_like<_Start>) {
            return difference_type(difference_type(__x.__value_) - difference_type(__y.__value_));
          }
          if (__y.__value_ > __x.__value_) {
            return difference_type(-difference_type(__y.__value_ - __x.__value_));
          }
          return difference_type(__x.__value_ - __y.__value_);
        }
        return __x.__value_ - __y.__value_;
```
for which the Standard reference implementation is
```
if constexpr (is-integer-like<W>) {
  if constexpr (is-signed-integer-like<W>)
    return D(D(x.value_) - D(y.value_));
  else
    return (y.value_ > x.value_)
      ? D(-D(y.value_ - x.value_))
      : D(x.value_ - y.value_);
} else {
  return x.value_ - y.value_;
}
```
Personally I'd recommend sticking to the reference implementation (including the `?:`), to make it easy to verify that libc++ matches the Standard-mandated behavior (even though it is not obvious //what that behavior actually is!//). Alternatively, if we wanted to make it easy to //read// the code, one way to do that is to disentangle the nested ifs and turn it into a plain if-else-if ladder:
```
        if constexpr (!__integer_like<_Start>) {
          return __x.__value_ - __y.__value_;
        } else if constexpr (__signed_integer_like<_Start>) {
          return difference_type(difference_type(__x.__value_) - difference_type(__y.__value_));
        } else if (__y.__value_ > __x.__value_) {
          return difference_type(-difference_type(__y.__value_ - __x.__value_));
        } else {
          return difference_type(__x.__value_ - __y.__value_);
        }
```
Writing it out longhand this way really illustrates the insanity of the reference implementation: each branch puts its explicit casts to `difference_type` in different places! Particularly the first and fourth branches (in the if-else-if ladder version above) are gratuitously different: the first uses an implicit conversion, the last an explicit conversion. If these conversions can have observably different behavior, something is seriously broken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107396