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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:89-90
+11  // VAR1c
+
+c   // VAR2a
 CHECK-LABEL: USE DEF FMT IMPL MATCH
----------------
thopre wrote:
> jhenderson wrote:
> > I'm not following the purpose of the blank line here?
> The CHECK-NEXT: [[#VAR2a]] is trying to match c which is also found in "// VAR1c". The CHECK-NEXT thus fails because it found c after the previous match on the same line.
I see. I'm beginning to think the names of the variables need improving. Rather than have VAR1, VAR2, VAR1a etc, how about "SIGN", "UNSI" "UHEX", "LHEX", etc, possibly with additional context for their actual values. The CHECKs might also want an additional `{{^}}` to avoid matching things spuriously.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:385
+
+  AdditionResult = UnsignedTen + MinusTen;
+  ASSERT_THAT_EXPECTED(AdditionResult, Succeeded());
----------------
thopre wrote:
> jhenderson wrote:
> > thopre wrote:
> > > 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"
> > Where has this been addressed?
> I forgot to send the comments I had written. I hope that answers why I didn't do it.
I'm not sure it does, if I'm honest. An unsigned 10 plus a signed -11 should trigger the clause at line 174 in FileCheck.cpp, whilst the other way around triggers the clause at 170. They are distinctly interesting from the other cases that trigger those clauses because they should trigger overflows, if I've understood things right, rather than resulting in max uint64_t, but I don't think that's tested directly. The fact that this happens to call into other functions seems somewhat irrelevant from testing a distinct unit perspective - if you refactored the code to no longer use the same code path as something else that might be tested independently, you still want the test coverage.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:405
+  // operator- tests.
+  Expected<ExpressionValue> SubstractionResult = MinusTen - SignedTen;
+  ASSERT_THAT_EXPECTED(SubstractionResult, Succeeded());
----------------
jhenderson wrote:
> thopre wrote:
> > jhenderson wrote:
> > > What about `MinusTen - MinusTen`?
> > Ditto.
> Where has this been addressed?
It seems to me like `operator-` is a distinct unit, and its interface should be fully tested independently (see also my comments above). I believe this ends path ends up calling into `checkedSub` for which I can see no direct testing (does it handle double-negatives correctly, for example?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390