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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The latest changes are a big improvement on readability, thanks. Did you consider using TEST_P to factor things out further? That way you'd only need to specify the input and expected output values for each test case, with only a single TEST itself.



================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:444
+  // Test both positive values with underflow.
+  SubtractionResult = TenValue - MaxUint64Value;
+  expectError<OverflowError>("overflow error", SubtractionResult.takeError());
----------------
thopre wrote:
> 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).
(Just to be clear, I only used std::abs as an intended example, rather than literally what should have been written).


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:298
+static Expected<ExpressionValue> doValueOperation(T1 LeftValue, T2 RightValue,
+                                                  bool IsAddition) {
+  ExpressionValue LeftOperand(LeftValue);
----------------
Don't know how plausible it is, but did you consider passing in a `function_ref` to the `operator+`/`operator-` functions instead of the boolean? That might be useful in the future if support is extended to e.g. multiplication.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:365
+
+  // getSignedValue() tests.
+
----------------
I think these sorts of comments indicate a need to split the test up. The suggestion I have is one method == one (or more) TESTs.

Thus there'd be tests for `getAbsolute`, `getSignedValue` etc etc and I'd call them `ExpressionValueGetAbsolute`, `ExpressionValueGetSignedValue` etc.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:411
+
+  // Test max uint64)t.
+  expectValueEqual(ExpressionValue(MaxUint64).getAbsolute(), MaxUint64);
----------------
Typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390