[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:385
+
+  AdditionResult = UnsignedTen + MinusTen;
+  ASSERT_THAT_EXPECTED(AdditionResult, Succeeded());
----------------
jhenderson wrote:
> 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.
I've reworked the ExpressionValue storage which should simplify the codepath somewhat and added one testcase to cover for an overflow case that was not tested (I was relying on testing of checkedSub but as you pointed out testing code would risk to bitrot in case the operator code is changed).

Please let me know if that doesn't address all your concerns.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:405
+  // operator- tests.
+  Expected<ExpressionValue> SubstractionResult = MinusTen - SignedTen;
+  ASSERT_THAT_EXPECTED(SubstractionResult, Succeeded());
----------------
jhenderson wrote:
> 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?)
How about now? Whether the value is signed or not doesn't matter anymore, only whether it's negative which should allow to reduce the amount of overflow codepaths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390