[PATCH] D80915: [FileCheck] Implement * and / operators for ExpressionValue.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Oh, I thought when we were talking about operators, we were talking about the front end FileCheck ability to use them, like we have with addition and subtraction already! That would be useful too (particularly multiplication to allow for something like N*sizeof(struct) in the check patterns, but I'm okay waiting for it.



================
Comment at: llvm/lib/Support/FileCheck.cpp:254
+    // Result cannot be represented as int64_t.
+    if (*Result > (uint64_t)std::numeric_limits<int64_t>::max() + 1)
+      return make_error<OverflowError>();
----------------
We've tended to use `static_cast` rather than C-style casts in this code. Doesn't this check also assume 2's complement? I'd expect to see something involving the min of int64_t explicitly. The `operator-` code has some example dancing around that you could adapt to achive this, I think.


================
Comment at: llvm/lib/Support/FileCheck.cpp:286
+    // Result cannot be represented as int64_t.
+    if (ResultValue > (uint64_t)std::numeric_limits<int64_t>::max() + 1)
+      return make_error<OverflowError>();
----------------
Same comment as above - I think this assumes 2's complement, which we shouldn't be assuming.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:482
+TEST_F(FileCheckTest, ExpressionValueMultiplication) {
+  // Test negative and positive value with negative result.
+  expectOperationValueResult(operator*, -3, 10, -30);
----------------
I think you can delete the "with negative/positive result" bit of the comments, since it's obvious this is the case.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:487-490
+  expectOperationValueResult(operator*, static_cast<uint64_t>(MaxInt64) + 1, -1,
+                             MinInt64);
+  expectOperationValueResult(operator*, -1, static_cast<uint64_t>(MaxInt64) + 1,
+                             MinInt64);
----------------
Again, this makes the assumption of 2's complement.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:511
+  expectOperationValueResult(operator*, -10, MinInt64);
+}
+
----------------
Maybe worth testing multiplication by zero, with both ends of the range (MaxUint64/MinInt64).


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:518
+  expectOperationValueResult(operator/, MinInt64, 1, MinInt64);
+  expectOperationValueResult(operator/, static_cast<uint64_t>(MaxInt64)+1, -1,
+                             MinInt64);
----------------
Same as above - 2's complement assumption.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:530-531
+  // Test divide by zero.
+  expectOperationValueResult(operator/, -10, 0);
+  expectOperationValueResult(operator/, 10, 0);
+
----------------
Perhaps also worth test cases showing the result of 0/0, 0/10, and 0/-10.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:540
+  // Test negative and positive value.
+  EXPECT_EQ(ExpressionValue(5) == ExpressionValue(-3), 0);
+  EXPECT_EQ(ExpressionValue(5) != ExpressionValue(-3), 1);
----------------
Replace these with `EXPECT_TRUE`/`EXPECT_FALSE`.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:543
+  EXPECT_EQ(ExpressionValue(-2) == ExpressionValue(6), 0);
+  EXPECT_EQ(ExpressionValue(-2) != ExpressionValue(6), 1);
+
----------------
Also test two values that are identical except for positive/negative, e.g. 1 and -1.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:549
+  EXPECT_EQ(ExpressionValue(-3) == ExpressionValue(-3), 1);
+  EXPECT_EQ(ExpressionValue(-3) != ExpressionValue(-3), 0);
+
----------------
For completeness, you probably also want to compare values that are different, e.g. 3 and 7. Same for the positive case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80915