[llvm] b743c19 - [FileCheck] Turn errors into assert in valueFromStringRepr()

[Thomas Preud'homme via llvm-commits]

Author: Thomas Preud'homme
Date: 2023-08-07T21:27:44+01:00
New Revision: b743c19360a67af4a709bd839e8c80ad17f71a1c

URL: https://github.com/llvm/llvm-project/commit/b743c19360a67af4a709bd839e8c80ad17f71a1c
DIFF: https://github.com/llvm/llvm-project/commit/b743c19360a67af4a709bd839e8c80ad17f71a1c.diff

LOG: [FileCheck] Turn errors into assert in valueFromStringRepr()

getWildcardRegex() guarantees that only valid hex numbers are matched by
FileCheck numeric expressions. This commit therefore only asserts the
lack of parsing failure in valueFromStringRepr().

Depends On D154430

Reviewed By: arichardson

Differential Revision: https://reviews.llvm.org/D154431

Added: 
    

Modified: 
    llvm/lib/FileCheck/FileCheck.cpp
    llvm/lib/FileCheck/FileCheckImpl.h
    llvm/unittests/FileCheck/FileCheckTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/FileCheck/FileCheck.cpp b/llvm/lib/FileCheck/FileCheck.cpp
index 60a0d5982afe90c..49fda8fb63cd36c 100644
--- a/llvm/lib/FileCheck/FileCheck.cpp
+++ b/llvm/lib/FileCheck/FileCheck.cpp
@@ -134,15 +134,9 @@ static APInt toSigned(APInt AbsVal, bool Negative) {
   return Result;
 }
 
-Expected<APInt>
-ExpressionFormat::valueFromStringRepr(StringRef StrVal,
-                                      const SourceMgr &SM) const {
+APInt ExpressionFormat::valueFromStringRepr(StringRef StrVal,
+                                            const SourceMgr &SM) const {
   bool ValueIsSigned = Value == Kind::Signed;
-  // Both the FileCheck utility and library only call this method with a valid
-  // value in StrVal. This is guaranteed by the regex returned by
-  // getWildcardRegex() above. Only underflow and overflow errors can thus
-  // occur. However new uses of this method could be added in the future so
-  // the error message does not make assumptions about StrVal.
   bool Negative = StrVal.consume_front("-");
   bool Hex = Value == Kind::HexUpper || Value == Kind::HexLower;
   bool MissingFormPrefix =
@@ -150,10 +144,12 @@ ExpressionFormat::valueFromStringRepr(StringRef StrVal,
   (void)MissingFormPrefix;
   assert(!MissingFormPrefix && "missing alternate form prefix");
   APInt ResultValue;
-  bool ParseFailure = StrVal.getAsInteger(Hex ? 16 : 10, ResultValue);
-  if (ParseFailure)
-    return ErrorDiagnostic::get(SM, StrVal,
-                                "unable to represent numeric value");
+  [[maybe_unused]] bool ParseFailure =
+      StrVal.getAsInteger(Hex ? 16 : 10, ResultValue);
+  // Both the FileCheck utility and library only call this method with a valid
+  // value in StrVal. This is guaranteed by the regex returned by
+  // getWildcardRegex() above.
+  assert(!ParseFailure && "unable to represent numeric value");
   return toSigned(ResultValue, Negative);
 }
 
@@ -1176,10 +1172,8 @@ Pattern::MatchResult Pattern::match(StringRef Buffer,
 
     StringRef MatchedValue = MatchInfo[CaptureParenGroup];
     ExpressionFormat Format = DefinedNumericVariable->getImplicitFormat();
-    Expected<APInt> Value = Format.valueFromStringRepr(MatchedValue, SM);
-    if (!Value)
-      return MatchResult(TheMatch, Value.takeError());
-    DefinedNumericVariable->setValue(*Value, MatchedValue);
+    APInt Value = Format.valueFromStringRepr(MatchedValue, SM);
+    DefinedNumericVariable->setValue(Value, MatchedValue);
   }
 
   return MatchResult(TheMatch, Error::success());

diff  --git a/llvm/lib/FileCheck/FileCheckImpl.h b/llvm/lib/FileCheck/FileCheckImpl.h
index f5f8ea2ab856ba7..c15461684ea3924 100644
--- a/llvm/lib/FileCheck/FileCheckImpl.h
+++ b/llvm/lib/FileCheck/FileCheckImpl.h
@@ -96,11 +96,8 @@ struct ExpressionFormat {
   Expected<std::string> getMatchingString(APInt Value) const;
 
   /// \returns the value corresponding to string representation \p StrVal
-  /// according to the matching format represented by this instance or an error
-  /// with diagnostic against \p SM if \p StrVal does not correspond to a valid
-  /// and representable value.
-  Expected<APInt> valueFromStringRepr(StringRef StrVal,
-                                      const SourceMgr &SM) const;
+  /// according to the matching format represented by this instance.
+  APInt valueFromStringRepr(StringRef StrVal, const SourceMgr &SM) const;
 };
 
 /// Class to represent an overflow error that might result when manipulating a

diff  --git a/llvm/unittests/FileCheck/FileCheckTest.cpp b/llvm/unittests/FileCheck/FileCheckTest.cpp
index 15036660b816b31..e93ee8de01302ee 100644
--- a/llvm/unittests/FileCheck/FileCheckTest.cpp
+++ b/llvm/unittests/FileCheck/FileCheckTest.cpp
@@ -201,29 +201,16 @@ struct ExpressionFormatParameterisedFixture
     EXPECT_THAT_EXPECTED(MatchingString, Failed());
   }
 
-  Expected<APInt> getValueFromStringReprFailure(StringRef Str) {
-    StringRef BufferizedStr = bufferize(SM, Str);
-    return Format.valueFromStringRepr(BufferizedStr, SM);
-  }
-
   template <class T>
   void checkValueFromStringRepr(StringRef Str, T ExpectedVal) {
-    Expected<APInt> ResultValue = getValueFromStringReprFailure(Str);
-    ASSERT_THAT_EXPECTED(ResultValue, Succeeded())
-        << "Failed to get value from " << Str;
-    ASSERT_EQ(ResultValue->isNegative(), ExpectedVal < 0)
+    StringRef BufferizedStr = bufferize(SM, Str);
+    APInt ResultValue = Format.valueFromStringRepr(BufferizedStr, SM);
+    ASSERT_EQ(ResultValue.isNegative(), ExpectedVal < 0)
         << "Value for " << Str << " is not " << ExpectedVal;
-    if (ResultValue->isNegative())
-      EXPECT_EQ(ResultValue->getSExtValue(), static_cast<int64_t>(ExpectedVal));
+    if (ResultValue.isNegative())
+      EXPECT_EQ(ResultValue.getSExtValue(), static_cast<int64_t>(ExpectedVal));
     else
-      EXPECT_EQ(ResultValue->getZExtValue(),
-                static_cast<uint64_t>(ExpectedVal));
-  }
-
-  void checkValueFromStringReprFailure(
-      StringRef Str, StringRef ErrorStr = "unable to represent numeric value") {
-    Expected<APInt> ResultValue = getValueFromStringReprFailure(Str);
-    expectDiagnosticError(ErrorStr, ResultValue.takeError());
+      EXPECT_EQ(ResultValue.getZExtValue(), static_cast<uint64_t>(ExpectedVal));
   }
 };
 
@@ -321,7 +308,10 @@ TEST_P(ExpressionFormatParameterisedFixture, FormatValueFromStringRepr) {
 
   // Wrong casing is not tested because valueFromStringRepr() relies on
   // StringRef's getAsInteger() which does not allow to restrict casing.
-  checkValueFromStringReprFailure(addBasePrefix("G"));
+
+  // Likewise, wrong letter digit for hex value is not tested because it is
+  // only caught by an assert in FileCheck due to getWildcardRegex()
+  // guaranteeing only valid letter digits are used.
 }
 
 TEST_P(ExpressionFormatParameterisedFixture, FormatBoolOperator) {