[PATCH] D13612: [IR] Add a `makeNoWrapRegion` method to `ConstantRange`

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: include/llvm/IR/ConstantRange.h:89-93
@@ +88,7 @@
+  /// Example:
+  ///  makeNoWrapRegion(Add, i8 1, AddOperator::NoSignedWrap) == [-128, 127)
+  ///  makeNoWrapRegion(Add, i8 1, AddOperator::NoUnsignedWrap) == [0, -1)
+  ///  makeNoWrapRegion(Add, i8 0, AddOperator::NoUnsignedWrap) == Full Set
+  static ConstantRange makeNoWrapRegion(Instruction::BinaryOps BinOp,
+                                        const APInt &C, bool SignedWrap);
+
----------------
nlewycky wrote:
> Example in comments don't match function declaration? Did you want to make it an 'int WrappingFlags'? (I see that you actually use it as a bool, but I really like the named flags API shown in the comment much more.)
I initially did have it as a `int WrappingFlags` (and I forgot to update the comments when I changed this to use a `bool`), but using `AddOperator::...` here seemed arbitrary (for instance, is it okay to have `BinOp` be `Instruction::Mul` and pass in `AddOperator::NoSignedWrap`?

However, I share your preference for named parameters :), so I'll happily change this back to use `AddOperator::NoUnsignedWrap` if you're okay with it.  I can also introduce an enum inside `ConstantRange` if that sounds better.


http://reviews.llvm.org/D13612