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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry for the delay in looking at this. Comments inline.



================
Comment at: llvm/lib/Support/FileCheck.cpp:47
+ExpressionFormat::getMatchingString(ExpressionValue IntegerValue) const {
+  Error ConvError = Value == Kind::Signed ? IntegerValue.convertSigned()
+                                          : IntegerValue.convertUnsigned();
----------------
I'd not abbreviate this name. It's not clear to me that "Conv" == "Conversion"

Also, it's more idiomatic to declare the Error within the if, to reduce its scope.


================
Comment at: llvm/lib/Support/FileCheck.cpp:91
+
+bool ExpressionValue::operator==(const ExpressionValue &other) {
+  if (Signed)
----------------
other -> Other


================
Comment at: llvm/lib/Support/FileCheck.cpp:92
+bool ExpressionValue::operator==(const ExpressionValue &other) {
+  if (Signed)
+    return SignedValue == other.SignedValue;
----------------
I think you need a check that `Signed == other.Signed`, as otherwise there will be some cases where the two values match but aren't of the same signed-ness (e.g. -1 and uint64_t max). Unless that's intentional of course.


================
Comment at: llvm/lib/Support/FileCheck.cpp:98
+
+Error ExpressionValue::convertSigned() {
+  if (Signed)
----------------
I'd make the names of this and `convertUnsigned` a bit more explicit, by adding "to", e.g. "toSigned" or "convertToSigned"


================
Comment at: llvm/lib/Support/FileCheck.cpp:986
                         [](const ErrorDiagnostic &E) {},
+                        [](const ValueError &E) {
+                          switch (E.getType()) {
----------------
As far as I can see, this block is just skipping the error? Why the switch/case?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:133
+  std::error_code convertToErrorCode() const override {
+    return inconvertibleErrorCode();
+  }
----------------
Perhaps a `not_supported` or `invalid_argument` would make more sense here.


================
Comment at: llvm/lib/Support/FileCheckImpl.h:158-162
+  /// Constructor for a signed value.
+  explicit ExpressionValue(int64_t Val) : SignedValue(Val), Signed(true) {}
+
+  /// Constructor for an unsigned value.
+  explicit ExpressionValue(uint64_t Val) : UnsignedValue(Val), Signed(false) {}
----------------
This pair of constructors feels like a recipe for confusion or compilation problems due to ambiguous resolution. I think it might be better to make Signed an additional parameter.


================
Comment at: llvm/lib/Support/FileCheckImpl.h:166-167
+  /// the same signedness and corresponding value.
+  bool operator==(const ExpressionValue &other);
+  bool operator!=(const ExpressionValue &other) { return !(*this == other); }
+
----------------
other -> Other


================
Comment at: llvm/lib/Support/FileCheckImpl.h:225-229
+  /// Constructor for a signed literal.
+  explicit ExpressionLiteral(int64_t Val) : Value(Val) {}
+
+  /// Constructor for an unsigned literal.
+  explicit ExpressionLiteral(uint64_t Val) : Value(Val) {}
----------------
As with above, this could lead to ambiguous compiler resolution, if Val is neither a uint64_t or int64_t. Perhaps not an issue in internal code, but it feels like there might be a better solution to me, perhaps using templates and std::is_signed in this case?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:291
 
-  /// Value of numeric variable, if defined, or None otherwise.
-  Optional<uint64_t> Value;
+  /// Value of numeric variable.
+  Optional<ExpressionValue> Value;
----------------
You've lost the reference to when this can be `None`. When can it be?


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:70
 10
+-30
+-29
----------------
Maybe there should be som testing for signed positive values here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390