[PATCH] D60390: FileCheck [10/12]: Add support for signed numeric values
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 19 01:36:09 PDT 2020
jhenderson added a comment.
I've been thinking more about test cases, and suggested a number more. If you can reduce the boiler plate for each individual test case, that would be great too. Perhaps even consider a TEST_P solution to specify input and expected output, whilst sharing the same code body.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:312
+ // Test failure with negative value.
+ ExpressionValue MinusTenValue(-10);
+ expectError<OverflowError>("overflow error",
----------------
I'd test the range edge of -1 instead of -10.
It may be worth showing it fails for int64_t min too.
Also, a 0 check would be good to ensure no off-by-1 errors/incorrect use of `<` versus `<=` etc.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:324-325
+ // Test failure with too big positive value.
+ expectError<OverflowError>("overflow error",
+ MaxUint64Value.getSignedValue().takeError());
+
----------------
Perhaps worth adding a int64_t max + 1 case.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:330
+ ASSERT_THAT_EXPECTED(SignedValue, Succeeded());
+ EXPECT_EQ(*SignedValue, -10);
+
----------------
Maybe also int64_t min and max cases?
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:339
+ ASSERT_THAT_EXPECTED(UnsignedValue, Succeeded());
+ EXPECT_EQ(*UnsignedValue, 10U);
+
----------------
Maybe worth considering test cases for 0, and uint64_t max too?
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:368
+ // Test both negative values with underflow.
+ AdditionResult = MinInt64Value + MinusTenValue;
+ expectError<OverflowError>("overflow error", AdditionResult.takeError());
----------------
Perhaps use -1 instead of -10.
Perhaps also `MinInt64 - MinInt64`.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:373-377
+ ASSERT_THAT_EXPECTED(AdditionResult, Succeeded());
+ EXPECT_FALSE(AdditionResult->isNegative());
+ Expected<int64_t> AdditionUnsignedValue = AdditionResult->getUnsignedValue();
+ ASSERT_THAT_EXPECTED(AdditionUnsignedValue, Succeeded());
+ EXPECT_EQ(*AdditionUnsignedValue, 0U);
----------------
These checks all have a lot of repeated code. It's probably time to factor out much of this into a function. It might also be a good idea to factor them into separate TESTs to allow the rest of the test to run in the event of a failure with an ASSERT etc, and to provide finer-grained test failures.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:385
+ ASSERT_THAT_EXPECTED(AdditionUnsignedValue, Succeeded());
+ EXPECT_EQ(*AdditionUnsignedValue, 0U);
+
----------------
I'd prefer these slightly to not just always give a result of 0. I think it is more interesting to show that 10 + -11 can produce a negative number, for example, whilst 11 + -10 produces a positive one. Same goes for the case above.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:396
+ // Test both positive values with overflow.
+ AdditionResult = MaxUint64Value + TenValue;
+ expectError<OverflowError>("overflow error", AdditionResult.takeError());
----------------
Perhaps 1 instead of 10 here.
Perhaps also `MaxUint64Value + MaxUint64Value` would be a useful case, to maximise surface area.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:401
+
+ // Test negative value and too big positive value.
+ Expected<ExpressionValue> SubtractionResult = MinusTenValue - MaxUint64Value;
----------------
What is meant by "too big" here? Be more precise in this comment.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:406
+ // Test negative and positive value with underflow.
+ SubtractionResult = MinInt64Value - TenValue;
+ expectError<OverflowError>("overflow error", SubtractionResult.takeError());
----------------
`- 1` rather than `- 10` would probably be best here.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:419
+ // Test both negative values.
+ SubtractionResult = MinusTenValue - MinusTenValue;
+ ASSERT_THAT_EXPECTED(SubtractionResult, Succeeded());
----------------
Similar to the + case, it might be nice for this to result in a non-zero value.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:434
+
+ // Test both positive values with result positive or nul.
+ ExpressionValue FiveValue(5);
----------------
"or nul"?
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:435
+ // Test both positive values with result positive or nul.
+ ExpressionValue FiveValue(5);
+ SubtractionResult = TenValue - FiveValue;
----------------
I'm not sure `Value` is a useful suffix to all these names. It wasn't confusing to me before either.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:444
+ // Test both positive values with underflow.
+ SubtractionResult = TenValue - MaxUint64Value;
+ expectError<OverflowError>("overflow error", SubtractionResult.takeError());
----------------
Maybe Zero instead of Ten here would be good, for maximum possible underflow.
Maybe consider `0 - (std::abs(MinInt64) + 1)` for minimum possible underflow.
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:455
+ ASSERT_THAT_EXPECTED(SubtractionSignedValue, Succeeded());
+ EXPECT_LT(*SubtractionSignedValue, -MaxInt64);
+
----------------
Why not test the exact value?
================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:458
+ // Test both positive values with result > -(max int64_t)
+ SubtractionResult = TenValue - TenValue;
+ ASSERT_THAT_EXPECTED(SubtractionResult, Succeeded());
----------------
Seems to me like there are two interesting cases here: "result is positive" and "result is negative".
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60390/new/
https://reviews.llvm.org/D60390
More information about the llvm-commits
mailing list