[PATCH] D59193: [ConstantRange] Add overflow check helpers

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Looks good, some nits.



================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:1315-1316
+
+          bool HasOverflow = false;
+          bool HasNoOverflow = false;
+          APInt N1(Bits, Lo1);
----------------
On the first look this is confusing.
Maybe add `Set`/`Range` prefix, so it is clearer that it is over the whole set, not just some one point.



================
Comment at: llvm/unittests/IR/ConstantRangeTest.cpp:1317-1329
+          APInt N1(Bits, Lo1);
+          for (unsigned I1 = 0; I1 < Size1; I1++, N1++) {
+            APInt N2(Bits, Lo2);
+            for (unsigned I2 = 0; I2 < Size2; I2++, N2++) {
+              assert(CR1.contains(N1));
+              assert(CR2.contains(N2));
+
----------------
Hm, so this is iterating so that `N1` is every point in `CR1` and `N2` is every point in `CR2`.
Took me a moment to get that.
Can this perhaps be rewritten with for-loop somehow, more explicitly?
```
for(APInt N1 = CR1.getLower(), APInt N1Max = CR1.getUpper(); N1 != N1Max; N1++) {
  for(APInt N2 = CR2.getLower(), APInt N2Max = CR2.getUpper(); N2 != N2Max; N2++) {
    assert(CR1.contains(N1));
    assert(CR2.contains(N2));

    if (OverflowFn(N1, N2))
      HasOverflow = true;
    else
       HasNoOverflow = true;
  }
}
```


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

https://reviews.llvm.org/D59193