[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/docs/CommandGuide/FileCheck.rst:697-699
+  Evaluation of an expression follows the C language integer promotion rules,
+  i.e. a signed 64-bit operand is promoted to an unsigned 64-bit operand if the
+  other operand is unsigned.
----------------
Is this still true?


================
Comment at: llvm/lib/Support/FileCheck.cpp:201
+  else
+    return ExpressionValue(-static_cast<int64_t>(RightValue - LeftValue),
+                           /*Negative=*/true);
----------------
I think there's an overflow issue here, right? If `RightValue` is bigger than max int64_t, it's possible for the expression to end up outside a representable int64_t, which seems like it should be an error?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:127-131
+  template <class T,
+            typename std::enable_if_t<std::is_signed<T>::value, int> = 0>
+  explicit ExpressionValue(T Val, bool Negative)
+      // Sign of value is unknown.
+      : Value(Val), Negative(Negative) {}
----------------
I'm looking at this and thinking can't this and the above constructor not just be changed to:

```
template <class T>
explicit ExpressionValue(T Val)
  : Value(Val), Negative(Val < 0) {}
```


================
Comment at: llvm/lib/Support/FileCheckImpl.h:133
+
+  /// Returns true is value is signed and negative, false otherwise.
+  bool isNegative() const { return Negative; }
----------------
true is -> true if


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:385
+
+  AdditionResult = UnsignedTen + MinusTen;
+  ASSERT_THAT_EXPECTED(AdditionResult, Succeeded());
----------------
thopre wrote:
> 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.
I'm starting to get a bit of review fatigue, so I'm starting to miss things, but if I've followed things right, my suggested simplification to the constructors should enable us to get rid of the distinction between SignedTen and UnsignedTen, which in turn should allow the tests to be much simpler. At that point, I'd expect the test cases to be:
1) Negative + Positive
2) Positive + Negative
3) Negative + Negative (no overflow)
4) Positive + Positive (no overflow)
5-6) Overflowing variations of 3) and 4)

with an equivalent set for operator-


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390