[llvm] r334634 - Change checked arithmetic functions API to return Optional
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 13 12:32:41 PDT 2018
The comments on the new functions are somewhat confusing. I think the
semantics you implement is that a Some result is returned if overflow
did not occur and that a None result is returned if it did, right?
If so, can you rephrase the function comments to state that explicitly?
Something along the following would be good: "Add two numbers and return
the result if no (signed *or* unsigned) overflow occurred. Otherwise,
return None."
The specific bit in your wording I find confusing is the "return wrapped
result if available". I don't know what that means.
On 06/13/2018 11:31 AM, George Karpenkov via llvm-commits wrote:
> Author: george.karpenkov
> Date: Wed Jun 13 11:31:43 2018
> New Revision: 334634
>
> URL: http://llvm.org/viewvc/llvm-project?rev=334634&view=rev
> Log:
> Change checked arithmetic functions API to return Optional
>
> Returning optional is much safer.
> The previous API had potential to cause use of undefined variables, if
> the value passed by pointer was accidentally read afterwards.
>
> Differential Revision: https://reviews.llvm.org/D48137
>
> Modified:
> llvm/trunk/include/llvm/Support/CheckedArithmetic.h
> llvm/trunk/unittests/Support/CheckedArithmeticTest.cpp
>
> Modified: llvm/trunk/include/llvm/Support/CheckedArithmetic.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/CheckedArithmetic.h?rev=334634&r1=334633&r2=334634&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/CheckedArithmetic.h (original)
> +++ llvm/trunk/include/llvm/Support/CheckedArithmetic.h Wed Jun 13 11:31:43 2018
> @@ -16,66 +16,61 @@
> #define LLVM_SUPPORT_CHECKEDARITHMETIC_H
>
> #include "llvm/ADT/APInt.h"
> +#include "llvm/ADT/Optional.h"
>
> #include <type_traits>
>
> namespace {
>
> /// Utility function to apply a given method of \c APInt \p F to \p LHS and
> -/// \p RHS, and write the output into \p Res.
> -/// \return Whether the operation overflows.
> +/// \p RHS.
> +/// \return Empty optional if the operation overflows, or result otherwise.
> template <typename T, typename F>
> typename std::enable_if<std::is_integral<T>::value && sizeof(T) * 8 <= 64,
> - bool>::type
> -checkedOp(T LHS, T RHS, F Op, T *Res = nullptr, bool Signed = true) {
> + llvm::Optional<T>>::type
> +checkedOp(T LHS, T RHS, F Op, bool Signed = true) {
> llvm::APInt ALHS(/*BitSize=*/sizeof(T) * 8, LHS, Signed);
> llvm::APInt ARHS(/*BitSize=*/sizeof(T) * 8, RHS, Signed);
> bool Overflow;
> llvm::APInt Out = (ALHS.*Op)(ARHS, Overflow);
> - if (Res)
> - *Res = Signed ? Out.getSExtValue() : Out.getZExtValue();
> - return Overflow;
> + if (Overflow)
> + return llvm::None;
> + return Signed ? Out.getSExtValue() : Out.getZExtValue();
> }
> }
>
> namespace llvm {
>
> -/// Add two signed integers \p LHS and \p RHS, write into \p Res if non-null.
> -/// Does not guarantee saturating arithmetic.
> -/// \return Whether the result overflows.
> +/// Add two signed integers \p LHS and \p RHS, return wrapped result if
> +/// available.
> template <typename T>
> -typename std::enable_if<std::is_signed<T>::value, bool>::type
> -checkedAdd(T LHS, T RHS, T *Res = nullptr) {
> - return checkedOp(LHS, RHS, &llvm::APInt::sadd_ov, Res);
> +typename std::enable_if<std::is_signed<T>::value, llvm::Optional<T>>::type
> +checkedAdd(T LHS, T RHS) {
> + return checkedOp(LHS, RHS, &llvm::APInt::sadd_ov);
> }
>
> -/// Multiply two signed integers \p LHS and \p RHS, write into \p Res if
> -/// non-null.
> -/// Does not guarantee saturating arithmetic.
> -/// \return Whether the result overflows.
> +/// Multiply two signed integers \p LHS and \p RHS, return wrapped result
> +/// if available.
> template <typename T>
> -typename std::enable_if<std::is_signed<T>::value, bool>::type
> -checkedMul(T LHS, T RHS, T *Res = nullptr) {
> - return checkedOp(LHS, RHS, &llvm::APInt::smul_ov, Res);
> +typename std::enable_if<std::is_signed<T>::value, llvm::Optional<T>>::type
> +checkedMul(T LHS, T RHS) {
> + return checkedOp(LHS, RHS, &llvm::APInt::smul_ov);
> }
>
> -/// Add two unsigned integers \p LHS and \p RHS, write into \p Res if non-null.
> -/// Does not guarantee saturating arithmetic.
> -/// \return Whether the result overflows.
> +/// Add two unsigned integers \p LHS and \p RHS, return wrapped result
> +/// if available.
> template <typename T>
> -typename std::enable_if<std::is_unsigned<T>::value, bool>::type
> -checkedAddUnsigned(T LHS, T RHS, T *Res = nullptr) {
> - return checkedOp(LHS, RHS, &llvm::APInt::uadd_ov, Res, /*Signed=*/false);
> +typename std::enable_if<std::is_unsigned<T>::value, llvm::Optional<T>>::type
> +checkedAddUnsigned(T LHS, T RHS) {
> + return checkedOp(LHS, RHS, &llvm::APInt::uadd_ov, /*Signed=*/false);
> }
>
> -/// Multiply two unsigned integers \p LHS and \p RHS, write into \p Res if
> -/// non-null.
> -/// Does not guarantee saturating arithmetic.
> -/// \return Whether the result overflows.
> +/// Multiply two unsigned integers \p LHS and \p RHS, return wrapped result
> +/// if available.
> template <typename T>
> -typename std::enable_if<std::is_unsigned<T>::value, bool>::type
> -checkedMulUnsigned(T LHS, T RHS, T *Res = nullptr) {
> - return checkedOp(LHS, RHS, &llvm::APInt::umul_ov, Res, /*Signed=*/false);
> +typename std::enable_if<std::is_unsigned<T>::value, llvm::Optional<T>>::type
> +checkedMulUnsigned(T LHS, T RHS) {
> + return checkedOp(LHS, RHS, &llvm::APInt::umul_ov, /*Signed=*/false);
> }
>
> } // End llvm namespace
>
> Modified: llvm/trunk/unittests/Support/CheckedArithmeticTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CheckedArithmeticTest.cpp?rev=334634&r1=334633&r2=334634&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/Support/CheckedArithmeticTest.cpp (original)
> +++ llvm/trunk/unittests/Support/CheckedArithmeticTest.cpp Wed Jun 13 11:31:43 2018
> @@ -6,65 +6,53 @@ using namespace llvm;
> namespace {
>
> TEST(CheckedArithmetic, CheckedAdd) {
> - int64_t Out;
> const int64_t Max = std::numeric_limits<int64_t>::max();
> const int64_t Min = std::numeric_limits<int64_t>::min();
> - EXPECT_EQ(checkedAdd<int64_t>(Max, Max, &Out), true);
> - EXPECT_EQ(checkedAdd<int64_t>(Min, -1, &Out), true);
> - EXPECT_EQ(checkedAdd<int64_t>(Max, 1, &Out), true);
> - EXPECT_EQ(checkedAdd<int64_t>(10, 1, &Out), false);
> - EXPECT_EQ(Out, 11);
> + EXPECT_EQ(checkedAdd<int64_t>(Max, Max), None);
> + EXPECT_EQ(checkedAdd<int64_t>(Min, -1), None);
> + EXPECT_EQ(checkedAdd<int64_t>(Max, 1), None);
> + EXPECT_EQ(checkedAdd<int64_t>(10, 1), Optional<int64_t>(11));
> }
>
> TEST(CheckedArithmetic, CheckedAddSmall) {
> - int16_t Out;
> const int16_t Max = std::numeric_limits<int16_t>::max();
> const int16_t Min = std::numeric_limits<int16_t>::min();
> - EXPECT_EQ(checkedAdd<int16_t>(Max, Max, &Out), true);
> - EXPECT_EQ(checkedAdd<int16_t>(Min, -1, &Out), true);
> - EXPECT_EQ(checkedAdd<int16_t>(Max, 1, &Out), true);
> - EXPECT_EQ(checkedAdd<int16_t>(10, 1, &Out), false);
> - EXPECT_EQ(Out, 11);
> + EXPECT_EQ(checkedAdd<int16_t>(Max, Max), None);
> + EXPECT_EQ(checkedAdd<int16_t>(Min, -1), None);
> + EXPECT_EQ(checkedAdd<int16_t>(Max, 1), None);
> + EXPECT_EQ(checkedAdd<int16_t>(10, 1), Optional<int64_t>(11));
> }
>
> TEST(CheckedArithmetic, CheckedMul) {
> - int64_t Out;
> const int64_t Max = std::numeric_limits<int64_t>::max();
> const int64_t Min = std::numeric_limits<int64_t>::min();
> - EXPECT_EQ(checkedMul<int64_t>(Max, 2, &Out), true);
> - EXPECT_EQ(checkedMul<int64_t>(Max, Max, &Out), true);
> - EXPECT_EQ(checkedMul<int64_t>(Min, 2, &Out), true);
> - EXPECT_EQ(checkedMul<int64_t>(10, 2, &Out), false);
> - EXPECT_EQ(Out, 20);
> + EXPECT_EQ(checkedMul<int64_t>(Max, 2), None);
> + EXPECT_EQ(checkedMul<int64_t>(Max, Max), None);
> + EXPECT_EQ(checkedMul<int64_t>(Min, 2), None);
> + EXPECT_EQ(checkedMul<int64_t>(10, 2), Optional<int64_t>(20));
> }
>
> TEST(CheckedArithmetic, CheckedMulSmall) {
> - int16_t Out;
> const int16_t Max = std::numeric_limits<int16_t>::max();
> const int16_t Min = std::numeric_limits<int16_t>::min();
> - EXPECT_EQ(checkedMul<int16_t>(Max, 2, &Out), true);
> - EXPECT_EQ(checkedMul<int16_t>(Max, Max, &Out), true);
> - EXPECT_EQ(checkedMul<int16_t>(Min, 2, &Out), true);
> - EXPECT_EQ(checkedMul<int16_t>(10, 2, &Out), false);
> - EXPECT_EQ(Out, 20);
> + EXPECT_EQ(checkedMul<int16_t>(Max, 2), None);
> + EXPECT_EQ(checkedMul<int16_t>(Max, Max), None);
> + EXPECT_EQ(checkedMul<int16_t>(Min, 2), None);
> + EXPECT_EQ(checkedMul<int16_t>(10, 2), Optional<int16_t>(20));
> }
>
> TEST(CheckedArithmetic, CheckedAddUnsigned) {
> - uint64_t Out;
> const uint64_t Max = std::numeric_limits<uint64_t>::max();
> - EXPECT_EQ(checkedAddUnsigned<uint64_t>(Max, Max, &Out), true);
> - EXPECT_EQ(checkedAddUnsigned<uint64_t>(Max, 1, &Out), true);
> - EXPECT_EQ(checkedAddUnsigned<uint64_t>(10, 1, &Out), false);
> - EXPECT_EQ(Out, uint64_t(11));
> + EXPECT_EQ(checkedAddUnsigned<uint64_t>(Max, Max), None);
> + EXPECT_EQ(checkedAddUnsigned<uint64_t>(Max, 1), None);
> + EXPECT_EQ(checkedAddUnsigned<uint64_t>(10, 1), Optional<uint64_t>(11));
> }
>
> TEST(CheckedArithmetic, CheckedMulUnsigned) {
> - uint64_t Out;
> const uint64_t Max = std::numeric_limits<uint64_t>::max();
> - EXPECT_EQ(checkedMulUnsigned<uint64_t>(Max, 2, &Out), true);
> - EXPECT_EQ(checkedMulUnsigned<uint64_t>(Max, Max, &Out), true);
> - EXPECT_EQ(checkedMulUnsigned<uint64_t>(10, 2, &Out), false);
> - EXPECT_EQ(Out, uint64_t(20));
> + EXPECT_EQ(checkedMulUnsigned<uint64_t>(Max, 2), llvm::None);
> + EXPECT_EQ(checkedMulUnsigned<uint64_t>(Max, Max), llvm::None);
> + EXPECT_EQ(checkedMulUnsigned<uint64_t>(10, 2), Optional<uint64_t>(20));
> }
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list