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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
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);
----------------
jhenderson wrote:
> 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.
Is that fine grain enough or do you want me to split further?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:444
+  // Test both positive values with underflow.
+  SubtractionResult = TenValue - MaxUint64Value;
+  expectError<OverflowError>("overflow error", SubtractionResult.takeError());
----------------
jhenderson wrote:
> Maybe Zero instead of Ten here would be good, for maximum possible underflow.
> Maybe consider `0 - (std::abs(MinInt64) + 1)` for minimum possible underflow.
I can't use std::abs(MinInt64) since it's undefined in 2-complement (std::abs returns an int64_t for some reason).


================
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());
----------------
jhenderson wrote:
> Seems to me like there are two interesting cases here: "result is positive" and "result is negative".
Result is positive is already handled above. I meant to handle the negative case where result < -(max int64_t). Fixed now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60390/new/

https://reviews.llvm.org/D60390