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

Tue May 12 02:39:19 PDT 2020

jhenderson added inline comments.

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:697
+  ``-``. Spaces are accepted before, after and between any of these elements.
+  Evaluation of an expression follows the C language integer promotion, i.e.
+  a signed 64-bit operand is promoted to an unsigned 64-bit operand if the
----------------
promotion rules

================
Comment at: llvm/lib/Support/FileCheck.cpp:110
+  if (Signed)
+    return APInt(sizeof(Value) * CHAR_BIT, Value, Signed).getSExtValue();
+
----------------
I'm a little confused as to what is going on here, as I'm not up-to-speed on APInt. Could you give a brief explanation, please?

================
Comment at: llvm/lib/Support/FileCheck.cpp:133
+
+  return ExpressionValue(~Value + 1, /*Signed=*/false);
+}
----------------
Doesn't this assume 2s complement, which isn't standard-guaranteed as far as I'm aware? I'm okay with that, if there is precendent for it in LLVM already, since it seems like C++ is moving in that direction (see http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r1.html), but I'd be nervous otherwise.

================
Comment at: llvm/lib/Support/FileCheck.cpp:150-156
+  // (-A) + B == B - A.
+  if (LeftOperand.isNegative())
+    return RightOperand - LeftOperand.getAbsolute();
+
+  // A + (-B) == A - B.
+  if (RightOperand.isNegative())
+    return LeftOperand - RightOperand.getAbsolute();
----------------
It might be worth a comment saying why signed plus unsigned results in an unsigned (of course, that assumes I'm following the logic correctly).

================
Comment at: llvm/lib/Support/FileCheck.cpp:915
+        Error Err =
+            handleErrors(Value.takeError(), [&](const OverflowError &E) {
+              return ErrorDiagnostic::get(SM, Substitution->getFromString(),
----------------
Why do we handle `OverflowError` here, but not other error kinds (e.g. undefined variables)?

================
Comment at: llvm/lib/Support/FileCheck.cpp:917
+              return ErrorDiagnostic::get(SM, Substitution->getFromString(),
+                                          "Unable to substitute variable or "
+                                          "numeric expression: overflow error");
----------------
Unable -> unable

(diagnostic messages should not be capitalized)

================
Comment at: llvm/lib/Support/FileCheckImpl.h:87
   /// \returns the string representation of \p Value in the format represented
-  /// by this instance, or an error if the format is NoFormat.
-  Expected<std::string> getMatchingString(uint64_t Value) const;
+  /// by this instance, or an error if conversion to this format failed or if
+  /// the format is NoFormat.
----------------
Delete the "if" at the end of this line (it's not needed).

================
Comment at: llvm/lib/Support/FileCheckImpl.h:106
+  std::error_code convertToErrorCode() const override {
+    return inconvertibleErrorCode();
+  }
----------------
`std::errc::value_too_large` is for overflows, so maybe that can be used here?

================
Comment at: llvm/lib/Support/FileCheckImpl.h:109
+
+  /// Print type of value conversion that failed.
+  void log(raw_ostream &OS) const override { OS << "Overflow error"; }
----------------
This comment doesn't seem right?

================
Comment at: llvm/lib/Support/FileCheckImpl.h:110
+  /// Print type of value conversion that failed.
+  void log(raw_ostream &OS) const override { OS << "Overflow error"; }
+};
----------------
Probably should be lower-case "overflow"

================
Comment at: llvm/lib/Support/FileCheckImpl.h:143
+
+/// Performs operation and \returns its result or an error in case of overflow.
+Expected<ExpressionValue> operator+(const ExpressionValue &lhs,
----------------
I'm slightly nervous "error in case of overflow" might go stale, in the event we introduce other failure modes. Perhaps "an error in case of a problem, such as an overflow."

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:41-43
+-30
 CHECK-LABEL: DEF FMT SPC
+CHECK-NEXT: [[# %d , VAR2a : ]]
----------------
Any particular reason you've changed this case?

================
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
----------------
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?

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:161
 1a
+1a
 D
----------------
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?

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:183
 CHECK-NEXT: [[# %X, VAR1]]
-CHECK-NEXT: [[# %u, VAR2]]
 CHECK-NEXT: [[# %u, VAR3]]
----------------
It seems like we need testing showing the behaviour of signed decimals converting to hex.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:415-416
+; Numeric expression with overflow.
+RUN: not FileCheck -check-prefix OVERFLOW -input-file %s %s 2>&1 \
+RUN:   | FileCheck -check-prefix OVERFLOW-MSG --strict-whitespace %s
+
----------------
Here and elsewhere, minor nit: be consistent with single or double-dashes.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:108
+  // Matches negative digits.
+  StringRef MinusFourtyTwo = "-42";
+  bool MatchSuccess = WildcardRegex.match(MinusFourtyTwo, &Matches);
----------------
MinusFortyTwo

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:385
+
+  AdditionResult = UnsignedTen + MinusTen;
+  ASSERT_THAT_EXPECTED(AdditionResult, Succeeded());
----------------
What about `10u + -11` or `-11 + 10u`?

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

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:461
   std::unique_ptr<ExpressionLiteral> Ten =
-      std::make_unique<ExpressionLiteral>(bufferize(SM, "10"), 10);
+      std::make_unique<ExpressionLiteral>(bufferize(SM, "10"), (uint64_t)10);
   ExpressionLiteral *TenPtr = Ten.get();
----------------
`10ull` would be more typical, I'd think, here and elsewhere. Any particular reason you've chosen the cast?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390