[llvm] r334634 - Change checked arithmetic functions API to return Optional
George Karpenkov via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 13 13:01:07 PDT 2018
You are right, I’ll do that.
> On Jun 13, 2018, at 12:32 PM, Philip Reames <listmail at philipreames.com> wrote:
>
> 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