Page MenuHomePhabricator

Utility functions for checked arithmetic
ClosedPublic

Authored by george.karpenkov on Feb 23 2018, 5:17 PM.

Details

Summary

Provide checkedAdd and checkedMul functions, providing checked arithmetic on signed 64 bit integers.

Diff Detail

Event Timeline

vsk added a comment.Feb 23 2018, 5:35 PM

It would be great to have checked* utilities which work with all the different kinds of integers.

include/llvm/Support/MathExtras.h
852

Once this lands, I think people will want to use it for more than just int64_t. To make this generic we'll need templates, and that introduces a cyclic-dependency problem with APInt.h.

Sorry for not thinking of this earlier, but: wdyt of moving this to Support/CheckedArithmetic.h (or similar)?

lib/Support/MathExtras.cpp
21

Could you move the APInt include up to the right spot?

26

Please clang-format your diffs.

29

llvm::function_ref is more appropriate for non-owning callables.

george.karpenkov marked 3 inline comments as done.Feb 23 2018, 7:04 PM
george.karpenkov added inline comments.
lib/Support/MathExtras.cpp
29

...except it does not work here, and I don't think it's worth it to investigate why

vsk added inline comments.Feb 26 2018, 1:34 PM
include/llvm/Support/CheckedArithmetic.h
31 ↗(On Diff #135765)

Can you move this out of the llvm namespace, into an anonymous namespace?

33 ↗(On Diff #135765)

I'd rather just have the type of the function be some free template parameter, then, so it's clear the object isn't copied.

include/llvm/Support/CheckedArithmetic.h
33 ↗(On Diff #135765)

Could you elaborate?

vsk added inline comments.Feb 26 2018, 1:44 PM
include/llvm/Support/CheckedArithmetic.h
33 ↗(On Diff #135765)

I'm suggesting adding 'typename F' to the template parameter list and using F as the type of Op. It's shorter, and since there's no std::function, it signals to the reader that there are no unnecessary copies happening. Taken together this makes the code easier to read.

george.karpenkov marked 2 inline comments as done.
vsk accepted this revision.Feb 26 2018, 2:10 PM

Thanks! Please wait a day or so before committing to give others time to provide feedback.

include/llvm/Support/CheckedArithmetic.h
20 ↗(On Diff #135972)

This include can go away now.

This revision is now accepted and ready to land.Feb 26 2018, 2:10 PM
This revision was automatically updated to reflect the committed changes.
This comment was removed by delcypher.