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 ↗(On Diff #135743)

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 ↗(On Diff #135743)

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

26 ↗(On Diff #135743)

Please clang-format your diffs.

29 ↗(On Diff #135743)

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 ↗(On Diff #135743)

...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
32

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

34

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
34

Could you elaborate?

vsk added inline comments.Feb 26 2018, 1:44 PM
include/llvm/Support/CheckedArithmetic.h
34

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

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.