[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/lib/Support/FileCheckImpl.h:127
+  bool isNegative() const {
+    return Signed && (Value & (1LLU << (sizeof(Value) * CHAR_BIT - 1)));
+  }
----------------
arichardson wrote:
> `Signed && (int64_t)Value < 0`?
Casting to int64_t is implementation-defined if Value is > std::numeric_limits<int64_t>::max(). That said current code assumes 2-complement so I've changed for memcpy. Hopefully the compiler can elide that call.


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:69
 USE DEF FMT IMPL MATCH
-11
-12
-10
-c
-d
-b
-1a
-D
-E
-C
-1B
-11
-11
-11
-c
-c
-c
-c
-c
+11  //VAR1
+12  //VAR1 + 1
----------------
jhenderson wrote:
> Nit: here and below, add space after comment markers, i.e. `// VAR1`
> 
> Also, has this been addressed:
> > Maybe there should be som testing for signed positive values here?
Yes, that's the VAR2+72 case.


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:161
 1a
+1a
 D
----------------
jhenderson wrote:
> Here and elsewhere there seem to be some extra cases added that aren't to do with signed decimal values. Do they belong in this change?
Ah yes indeed, I noticed some bug in the testcase. I forgot to split the fix out in its own patch. Now done in https://reviews.llvm.org/D79820


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:385
+
+  AdditionResult = UnsignedTen + MinusTen;
+  ASSERT_THAT_EXPECTED(AdditionResult, Succeeded());
----------------
jhenderson wrote:
> What about `10u + -11` or `-11 + 10u`?
Those are ExpressionValue since this test is testing ExpressionValue operator+. I've renamed all such variable to add the suffix "Value"


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:405
+  // operator- tests.
+  Expected<ExpressionValue> SubstractionResult = MinusTen - SignedTen;
+  ASSERT_THAT_EXPECTED(SubstractionResult, Succeeded());
----------------
jhenderson wrote:
> What about `MinusTen - MinusTen`?
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390