[FileCheck] Share variable instance among uses

[thopre]

Summary

This patch changes expression support to use one instance of
FileCheckNumericVariable per numeric variable rather than one per
variable and per definition. The current system was only necessary for
the last patch of the numeric expression support patch series in order
to handle a line using a variable defined earlier on the same line from
the input text. However this can be dealt more efficiently.

[jhenderson]

Thanks! I think this makes more sense now too. LGTM.

llvm/lib/Support/FileCheck.cpp

260

Nit: is this required here?

[thopre]

Address review comments

[thopre]

Also share the @LINE numeric variable class instance

[jhenderson]

LGTM, with one nit.

llvm/include/llvm/Support/FileCheck.h

305

when pattern are -> when a pattern is | when patterns are

llvm/include/llvm/Support/FileCheck.h

struct FileCheckRequest {
   bool Verbose = false;
   bool VerboseVerbose = false;
 };
 
 //===----------------------------------------------------------------------===//
 // Numeric substitution handling code.
 //===----------------------------------------------------------------------===//
 
-/// Class representing a numeric variable with a given value in a numeric
-/// expression. Each definition of a variable gets its own instance of this
-/// class. Variable uses share the same instance as their respective
-/// definition.
+/// Class representing a numeric variable and its associated current value.
 class FileCheckNumericVariable {
 private:
   /// Name of the numeric variable.
   StringRef Name;
 
   /// Value of numeric variable, if defined, or None otherwise.
   Optional<uint64_t> Value;
 
private:
   StringMap<StringRef> GlobalVariableTable;
 
   /// Map of all string variables defined so far. Used at parse time to detect
   /// a name conflict between a numeric variable and a string variable when
   /// the former is defined on a later line than the latter.
   StringMap<bool> DefinedVariableTable;
 
   /// When matching a given pattern, this holds the pointers to the classes
-  /// representing the last definitions of numeric variables defined in
-  /// previous patterns. Earlier definitions of the variables, if any, have
-  /// their own class instance not referenced by this table. When matching a
-  /// pattern all definitions for that pattern are recorded in the
+  /// representing the numeric variables defined in previous patterns. When
+  /// matching a pattern all definitions for that pattern are recorded in the
   /// NumericVariableDefs table in the FileCheckPattern instance of that
   /// pattern.
   StringMap<FileCheckNumericVariable *> GlobalNumericVariableTable;
 
+  /// Pointer to the class instance representing the @LINE pseudo variable for
+  /// easily updating its value.
+  FileCheckNumericVariable *LineVariable = nullptr;
+
   /// Vector holding pointers to all parsed expressions. Used to automatically
   /// free the expressions once they are guaranteed to no longer be used.
   std::vector<std::unique_ptr<FileCheckExpression>> Expressions;
 
   /// Vector holding pointers to all parsed numeric variables. Used to
   /// automatically free them once they are guaranteed to no longer be used.
   std::vector<std::unique_ptr<FileCheckNumericVariable>> NumericVariables;
 
   /// Vector holding pointers to all substitutions. Used to automatically free
   /// them once they are guaranteed to no longer be used.
   std::vector<std::unique_ptr<FileCheckSubstitution>> Substitutions;
 
 public:
   /// \returns the value of string variable \p VarName or an error if no such
   /// variable has been defined.
   Expected<StringRef> getPatternVarValue(StringRef VarName);
 
   /// Defines string and numeric variables from definitions given on the
   /// command line, passed as a vector of [#]VAR=VAL strings in
   /// \p CmdlineDefines. \returns an error list containing diagnostics against
   /// \p SM for all definition parsing failures, if any, or Success otherwise.
   Error defineCmdlineVariables(std::vector<std::string> &CmdlineDefines,
                                SourceMgr &SM);
 
+  /// Create @LINE pseudo variable. Value is set when pattern are being
+  /// matched.
+  void createLineVariable();
+
   /// Undefines local variables (variables whose name does not start with a '$'
   /// sign), i.e. removes them from GlobalVariableTable and from
   /// GlobalNumericVariableTable and also clears the value of numeric
   /// variables.
   void clearLocalVars();
 
 private:
   /// Makes a new expression instance and registers it for destruction when
public:
   static bool isValidVarNameStart(char C);
   /// Parses the string at the start of \p Str for a variable name. \returns
   /// an error holding a diagnostic against \p SM if parsing fail, or the
   /// name of the variable otherwise. In the latter case, sets \p IsPseudo to
   /// indicate if it is a pseudo variable and strips \p Str from the variable
   /// name.
   static Expected<StringRef> parseVariable(StringRef &Str, bool &IsPseudo,
                                            const SourceMgr &SM);
-  /// Parses \p Expr for the name of a numeric variable to be defined. \returns
-  /// an error holding a diagnostic against \p SM should defining such a
-  /// variable be invalid, or Success otherwise. In the latter case, sets
-  /// \p Name to the name of the parsed numeric variable name.
-  static Error parseNumericVariableDefinition(StringRef &Expr, StringRef &Name,
-                                              FileCheckPatternContext *Context,
-                                              const SourceMgr &SM);
+  /// Parses \p Expr for the name of a numeric variable to be defined at line
+  /// \p LineNumber. \returns a pointer to the class instance representing that
+  /// variable, creating it if needed, or an error holding a diagnostic against
+  /// \p SM should defining such a variable be invalid.
+  static Expected<FileCheckNumericVariable *>
+  parseNumericVariableDefinition(StringRef &Expr,
+                                 size_t LineNumber, const SourceMgr &SM);
   /// Parses \p Expr for a numeric substitution block. \returns the class
   /// representing the AST of the expression whose value must be substituted,
   /// or an error holding a diagnostic against \p SM if parsing fails. If
   /// substitution was successful, sets \p DefinedNumericVariable to point to
   /// the class representing the numeric variable defined in this numeric
   /// substitution block, or None if this block does not define any variable.
   Expected<FileCheckExpression *> parseNumericSubstitutionBlock(
       StringRef Expr,

llvm/lib/Support/FileCheck.cpp

static char popFront(StringRef &S) {
   S = S.drop_front();
   return C;
 }
 
 char FileCheckUndefVarError::ID = 0;
 char FileCheckErrorDiagnostic::ID = 0;
 char FileCheckNotFoundError::ID = 0;
 
-Error FileCheckPattern::parseNumericVariableDefinition(
-    StringRef &Expr, StringRef &Name, FileCheckPatternContext *Context,
+Expected<FileCheckNumericVariable *>
+FileCheckPattern::parseNumericVariableDefinition(
+    StringRef &Expr, FileCheckPatternContext *Context, size_t LineNumber,
     const SourceMgr &SM) {
   bool IsPseudo;
   Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
   if (!ParseVarResult)
     return ParseVarResult.takeError();
-  Name = *ParseVarResult;
+  StringRef Name = *ParseVarResult;
 
   if (IsPseudo)
     return FileCheckErrorDiagnostic::get(
         SM, Name, "definition of pseudo numeric variable unsupported");
 
   // Detect collisions between string and numeric variables when the latter
   // is created later than the former.
   if (Context->DefinedVariableTable.find(Name) !=
       Context->DefinedVariableTable.end())
     return FileCheckErrorDiagnostic::get(
         SM, Name, "string variable with name '" + Name + "' already exists");
 
   Expr = Expr.ltrim(SpaceChars);
   if (!Expr.empty())
     return FileCheckErrorDiagnostic::get(
         SM, Expr, "unexpected characters after numeric variable name");
 
-  return Error::success();
+  FileCheckNumericVariable *DefinedNumericVariable;
+  auto VarTableIter = Context->GlobalNumericVariableTable.find(Name);
+  if (VarTableIter != Context->GlobalNumericVariableTable.end())
+    DefinedNumericVariable = VarTableIter->second;
+  else
+    DefinedNumericVariable = Context->makeNumericVariable(LineNumber, Name);
+
+  return DefinedNumericVariable;
 }
 
 Expected<FileCheckNumericVariable *>
 FileCheckPattern::parseNumericVariableUse(StringRef &Expr,
                                           const SourceMgr &SM) const {
   bool IsPseudo;
   Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
   if (!ParseVarResult)
     return ParseVarResult.takeError();
   StringRef Name = *ParseVarResult;
 
   if (IsPseudo && !Name.equals("@LINE"))
     return FileCheckErrorDiagnostic::get(
         SM, Name, "invalid pseudo numeric variable '" + Name + "'");
 
   // Numeric variable definitions and uses are parsed in the order in which
   // they appear in the CHECK patterns. For each definition, the pointer to the
   // class instance of the corresponding numeric variable definition is stored
-  // in GlobalNumericVariableTable in parsePattern. Therefore, the pointer we
-  // get below is for the class instance corresponding to the last definition
-  // of this variable use. If we don't find a variable definition we create a
-  // dummy one so that parsing can continue. All uses of undefined variables,
-  // whether string or numeric, are then diagnosed in printSubstitutions()
-  // after failing to match.
+  // in GlobalNumericVariableTable in parsePattern. Therefore, if the pointer
+  // we get below is null, it means no such variable was defined before. When
+  // that happens, we create a dummy variable so that parsing can continue. All
+  // uses of undefined variables, whether string or numeric, are then diagnosed
+  // in printSubstitutions() after failing to match.
   auto VarTableIter = Context->GlobalNumericVariableTable.find(Name);
   FileCheckNumericVariable *NumericVariable;
   if (VarTableIter != Context->GlobalNumericVariableTable.end())
     NumericVariable = VarTableIter->second;
   else {
     NumericVariable = Context->makeNumericVariable(0, Name);
     Context->GlobalNumericVariableTable[Name] = NumericVariable;
   }
if (DefEnd != StringRef::npos) {
 
     UseExpr = UseExpr.ltrim(SpaceChars);
     if (!UseExpr.empty())
       return FileCheckErrorDiagnostic::get(
           SM, UseExpr,
           "unexpected string after variable definition: '" + UseExpr + "'");
 
     DefExpr = DefExpr.ltrim(SpaceChars);
-    StringRef Name;
-    Error Err = parseNumericVariableDefinition(DefExpr, Name, Context, SM);
-    if (Err)
-      return std::move(Err);
-    DefinedNumericVariable = Context->makeNumericVariable(LineNumber, Name);
+    Expected<FileCheckNumericVariable *> ParseResult =
+        parseNumericVariableDefinition(DefExpr, Context, LineNumber, SM);
+    if (!ParseResult)
+      return ParseResult.takeError();
+    DefinedNumericVariable = *ParseResult;
 
     return Context->makeExpression(add, nullptr, 0);
   }
 
   // Parse the expression itself.
   Expr = Expr.ltrim(SpaceChars);
   return parseBinop(Expr, SM);
 }
 
 bool FileCheckPattern::parsePattern(StringRef PatternStr, StringRef Prefix,
                                     SourceMgr &SM,
                                     const FileCheckRequest &Req) {
   bool MatchFullLinesHere = Req.MatchFullLines && CheckTy != Check::CheckNot;
 
   PatternLoc = SMLoc::getFromPointer(PatternStr.data());
 
-  // Create fake @LINE pseudo variable definition.
-  StringRef LinePseudo = "@LINE";
-  uint64_t LineNumber64 = LineNumber;
-  FileCheckNumericVariable *LinePseudoVar =
-      Context->makeNumericVariable(LinePseudo, LineNumber64);
-  Context->GlobalNumericVariableTable[LinePseudo] = LinePseudoVar;
-
   if (!(Req.NoCanonicalizeWhiteSpace && Req.MatchFullLines))
     // Ignore trailing whitespace.
     while (!PatternStr.empty() &&
            (PatternStr.back() == ' ' || PatternStr.back() == '\t'))
       PatternStr = PatternStr.substr(0, PatternStr.size() - 1);
 
   // Check that there is something on the line.
   if (PatternStr.empty() && CheckTy != Check::CheckEmpty) {
Expected<size_t> FileCheckPattern::match(StringRef Buffer, size_t &MatchLen,
   // Regex match.
 
   // If there are substitutions, we need to create a temporary string with the
   // actual value.
   StringRef RegExToMatch = RegExStr;
   std::string TmpStr;
   if (!Substitutions.empty()) {
     TmpStr = RegExStr;
+    Context->LineVariable->setValue(LineNumber);
 
     size_t InsertOffset = 0;
     // Substitute all string variables and expressions whose values are only
     // now known. Use of string variables defined on the same line are handled
     // by back-references.
     for (const auto &Substitution : Substitutions) {
       // Substitute and check for failure (e.g. use of undefined variable).
       Expected<std::string> Value = Substitution->getResult();
       if (!Value)
         return Value.takeError();
 
       // Plop it into the regex at the adjusted offset.
       TmpStr.insert(TmpStr.begin() + Substitution->getIndex() + InsertOffset,
                     Value->begin(), Value->end());
       InsertOffset += Value->size();
     }
 
     // Match the newly constructed regex.
     RegExToMatch = TmpStr;
+    Context->LineVariable->clearValue();
   }
 
   SmallVector<StringRef, 4> MatchInfo;
   if (!Regex(RegExToMatch, Regex::Newline).match(Buffer, &MatchInfo))
     return make_error<FileCheckNotFoundError>();
 
   // Successful regex match.
   assert(!MatchInfo.empty() && "Didn't get any match");
while (!Buffer.empty()) {
     // matched and any additional parts of that check-like word.
     Buffer = Buffer.drop_front(SkipWord(Buffer, Prefix.size()));
   }
 
   // We ran out of buffer while skipping partial matches so give up.
   return {StringRef(), StringRef()};
 }
 
+void FileCheckPatternContext::createLineVariable() {
+  assert(!LineVariable && "@LINE pseudo numeric variable already created");
+  StringRef LineName = "@LINE";
+  LineVariable = makeNumericVariable(0, LineName);
+  GlobalNumericVariableTable[LineName] = LineVariable;
+}
+
 bool FileCheck::ReadCheckFile(SourceMgr &SM, StringRef Buffer, Regex &PrefixRE,
                               std::vector<FileCheckString> &CheckStrings) {
   Error DefineError =
       PatternContext.defineCmdlineVariables(Req.GlobalDefines, SM);
   if (DefineError) {
     logAllUnhandledErrors(std::move(DefineError), errs());
     return true;
   }
 
+  PatternContext.createLineVariable();
+
   std::vector<FileCheckPattern> ImplicitNegativeChecks;
   for (const auto &PatternString : Req.ImplicitCheckNot) {
     // Create a buffer with fake command line content in order to display the
     // command line option responsible for the specific implicit CHECK-NOT.
     std::string Prefix = "-implicit-check-not='";
     std::string Suffix = "'";
     std::unique_ptr<MemoryBuffer> CmdLine = MemoryBuffer::getMemBufferCopy(
         Prefix + PatternString + Suffix, "command line");
if (EqIdx == StringRef::npos) {
           FileCheckErrorDiagnostic::get(
               SM, CmdlineDef, "missing equal sign in global definition"));
       continue;
     }
 
     // Numeric variable definition.
     if (CmdlineDef[0] == '#') {
       StringRef CmdlineName = CmdlineDef.substr(1, EqIdx - 1);
-      StringRef VarName;
-      Error ErrorDiagnostic = FileCheckPattern::parseNumericVariableDefinition(
-          CmdlineName, VarName, this, SM);
-      if (ErrorDiagnostic) {
-        Errs = joinErrors(std::move(Errs), std::move(ErrorDiagnostic));
+      Expected<FileCheckNumericVariable *> ParseResult =
+          FileCheckPattern::parseNumericVariableDefinition(CmdlineName, this, 0,
+                                                           SM);
+      if (!ParseResult) {
+        Errs = joinErrors(std::move(Errs), ParseResult.takeError());
         continue;
       }
 
       StringRef CmdlineVal = CmdlineDef.substr(EqIdx + 1);
       uint64_t Val;
       if (CmdlineVal.getAsInteger(10, Val)) {
         Errs = joinErrors(std::move(Errs),
                           FileCheckErrorDiagnostic::get(
                               SM, CmdlineVal,
                               "invalid value in numeric variable definition '" +
                                   CmdlineVal + "'"));
         continue;
       }
-      auto DefinedNumericVariable = makeNumericVariable(0, VarName);
+      FileCheckNumericVariable *DefinedNumericVariable = *ParseResult;
       DefinedNumericVariable->setValue(Val);
 
       // Record this variable definition.
       GlobalNumericVariableTable[DefinedNumericVariable->getName()] =
           DefinedNumericVariable;
     } else {
       // String variable definition.
       std::pair<StringRef, StringRef> CmdlineNameVal = CmdlineDef.split('=');

llvm/unittests/Support/FileCheckTest.cpp

 
 public:
   PatternTester() {
     std::vector<std::string> GlobalDefines;
     GlobalDefines.emplace_back(std::string("#FOO=42"));
     GlobalDefines.emplace_back(std::string("BAR=BAZ"));
     EXPECT_FALSE(
         errorToBool(Context.defineCmdlineVariables(GlobalDefines, SM)));
+    Context.createLineVariable();
     // Call parsePattern to have @LINE defined.
     P.parsePattern("N/A", "CHECK", SM, Req);
     // parsePattern does not expect to be called twice for the same line and
     // will set FixedStr and RegExStr incorrectly if it is. Therefore prepare
     // a pattern for a different line.
     initNextPattern();
   }
 
   void initNextPattern() {
     P = FileCheckPattern(Check::CheckPlain, &Context, LineNumber++);
   }
 
   bool parseNumVarDefExpect(StringRef Expr) {
     StringRef ExprBufferRef = bufferize(SM, Expr);
-    StringRef Name;
     return errorToBool(FileCheckPattern::parseNumericVariableDefinition(
-        ExprBufferRef, Name, &Context, SM));
+                           ExprBufferRef, &Context, LineNumber, SM)
+                           .takeError());
   }
 
   bool parseSubstExpect(StringRef Expr) {
     StringRef ExprBufferRef = bufferize(SM, Expr);
     Optional<FileCheckNumericVariable *> DefinedNumericVariable;
     return errorToBool(P.parseNumericSubstitutionBlock(
                             ExprBufferRef, DefinedNumericVariable, SM)
                            .takeError());