[PATCH] D60390: FileCheck [10/12]: Add support for signed numeric values

[James Henderson via Phabricator via llvm-commits]

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