[PATCH] D65494: [Support] Added overflow checking add, sub and mul.

[Nandor Licker via Phabricator via llvm-commits]

nand marked 4 inline comments as done.
nand added inline comments.


================
Comment at: llvm/include/llvm/Support/MathExtras.h:783-786
   // Hacker's Delight, p. 29
   T Z = X + Y;
   Overflowed = (Z < X || Z < Y);
   if (Overflowed)
----------------
lebedev.ri wrote:
> Do you want to also refactor the functions that already used that checking at the same time?
Unfortunately, I would like to avoid any refactoring in this diff.


================
Comment at: llvm/include/llvm/Support/MathExtras.h:862
+typename std::enable_if<std::is_signed<T>::value, T>::type
+AddOverflow(T X, T Y, T &Result) {
+  // Perform the unsigned addition.
----------------
jfb wrote:
> I think you want to have `T1 X, T2 Y` and `enable_if::<is_same<T1, T2>::value` since you're trying to avoid promotions entirely.
> 
> I'd also rather return a pair or a struct with named values instead of having a reference return value, and make the result `nodiscard`.
I would like to keep these functions simple so at some point we might make them straightforward wrappers over `__builtin_add_overflow` etc, reason why I would like to keep this signature. Promotions can be handled on the callee's side. For my use case, I always want to explicitly specify the template parameter type.

Regarding struct returns, that sounds like a use case for a class such as CheckedInt/SafeInt etc, which doesn't seem to be required yet. A future implementation might use these methods though.


================
Comment at: llvm/include/llvm/Support/MathExtras.h:870
+  // Bitcast to signed.
+  Result = static_cast<T>(UResult);
+
----------------
jfb wrote:
> We have `bit_cast` in `ADT/bit.h` if that's what you meant to use. I think you meant `static_cast` thought :)
If the sizes match, doesn't static cast simply move the number bit by bit to the result? I have adjusted the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65494