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

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

Can you confirm that the tests pass with UBSan enabled?



================
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.
----------------
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`.


================
Comment at: llvm/include/llvm/Support/MathExtras.h:870
+  // Bitcast to signed.
+  Result = static_cast<T>(UResult);
+
----------------
We have `bit_cast` in `ADT/bit.h` if that's what you meant to use. I think you meant `static_cast` thought :)


================
Comment at: llvm/unittests/Support/MathExtrasTest.cpp:474
+
+using OverflowTestTypes = ::testing::Types<char, short, int, long, long long>;
+
----------------
You probably want `signed char` instead of `char`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65494