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

[JF Bastien via Phabricator via llvm-commits]

jfb accepted this revision.
jfb added inline comments.
This revision is now accepted and ready to land.


================
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:
> > 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.
> This method is the only way to compute the result of overflow - if we know overflow happens, we might not want to look at the return flag at all. 
That sounds good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65494