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

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
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.
----------------
nand wrote:
> 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.
OK seeing this as the base helper SGTM.

You probably still want `nodiscard` however.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65494