[FileCheck] Store line numbers as optional values

[thopre]

Summary

Processing of command-line definition of variable and logic around
implicit not directives both reuse parsing code that expects a line
number to be defined. So far, a special line number of 0 was used for
those users of the parsing code where a line number does not make sense.
This commit instead represents line numbers as Optional values so that
they can be None for those cases.

[jhenderson]

Looks good, aside from one question.

llvm/lib/Support/FileCheck.cpp

186

Is the IsPseudo field here redundant now? Are there pseudo variables with a defined line number?

[thopre]

Address review comment

llvm/lib/Support/FileCheck.cpp

186

No indeed and even if there as it's irrelevant. What matters is whether the Definition line number is same the current line.

[jhenderson]

LGTM.

llvm/include/llvm/Support/FileCheck.h

 class FileCheckNumericVariable {
 private:
   /// Name of the numeric variable.
   StringRef Name;
 
   /// Value of numeric variable, if defined, or None otherwise.
   Optional<uint64_t> Value;
 
-  /// Line number where this variable is defined. Used to determine whether a
-  /// variable is defined on the same line as a given use.
-  size_t DefLineNumber;
+  /// Line number where this variable is defined, or None if defined before
+  /// input is parsed. Used to determine whether a variable is defined on the
+  /// same line as a given use.
+  Optional<size_t> DefLineNumber;
 
 public:
-  /// Constructor for a variable \p Name defined at line \p DefLineNumber.
-  FileCheckNumericVariable(size_t DefLineNumber, StringRef Name)
+  /// Constructor for a variable \p Name defined at line \p DefLineNumber or
+  /// defined before input is parsed if DefLineNumber is None.
+  FileCheckNumericVariable(StringRef Name,
+                           Optional<size_t> DefLineNumber = None)
       : Name(Name), DefLineNumber(DefLineNumber) {}
 
-  /// Constructor for numeric variable \p Name with a known \p Value at parse
-  /// time (e.g. the @LINE numeric variable).
-  FileCheckNumericVariable(StringRef Name, uint64_t Value)
-      : Name(Name), Value(Value), DefLineNumber(0) {}
-
   /// \returns name of this numeric variable.
   StringRef getName() const { return Name; }
 
   /// \returns this variable's value.
   Optional<uint64_t> getValue() const { return Value; }
 
   /// Sets value of this numeric variable, if undefined. Triggers an assertion
   /// failure if the variable is actually defined.
   void setValue(uint64_t Value);
 
   /// Clears value of this numeric variable, regardless of whether it is
   /// currently defined or not.
   void clearValue();
 
-  /// \returns the line number where this variable is defined.
-  size_t getDefLineNumber() { return DefLineNumber; }
+  /// \returns the line number where this variable is defined, if any, or None
+  /// if defined before input is parsed.
+  Optional<size_t> getDefLineNumber() { return DefLineNumber; }
 };
 
 /// Class representing the use of a numeric variable in the AST of an
 /// expression.
 class FileCheckNumericVariableUse : public FileCheckExpressionAST {
 private:
   /// Name of the numeric variable.
   StringRef Name;
class FileCheckPattern {
   /// - separate tables with the values of live string and numeric variables
   ///   respectively at the start of any given CHECK line;
   /// - table holding whether a string variable has been defined at any given
   ///   point during the parsing phase.
   FileCheckPatternContext *Context;
 
   Check::FileCheckType CheckTy;
 
-  /// Line number for this CHECK pattern. Used to determine whether a variable
-  /// definition is made on an earlier line to the one with this CHECK.
-  size_t LineNumber;
+  /// Line number for this CHECK pattern or None if it is an implicit pattern.
+  /// Used to determine whether a variable definition is made on an earlier
+  /// line to the one with this CHECK.
+  Optional<size_t> LineNumber;
 
 public:
   FileCheckPattern(Check::FileCheckType Ty, FileCheckPatternContext *Context,
-                   size_t Line)
+                   Optional<size_t> Line = None)
       : Context(Context), CheckTy(Ty), LineNumber(Line) {}
 
   /// \returns the location in source code.
   SMLoc getLoc() const { return PatternLoc; }
 
   /// \returns the pointer to the global state for all patterns in this
   /// FileCheck instance.
   FileCheckPatternContext *getContext() const { return Context; }
public:
   /// Parses the string at the start of \p Str for a variable name. \returns
   /// a VariableProperties structure holding the variable name and whether it
   /// is the name of a pseudo variable, or an error holding a diagnostic
   /// against \p SM if parsing fail. If parsing was successful, also strips
   /// \p Str from the variable name.
   static Expected<VariableProperties> parseVariable(StringRef &Str,
                                                     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,
-                                 FileCheckPatternContext *Context,
-                                 size_t LineNumber, const SourceMgr &SM);
+  /// \p LineNumber or before input is parsed if \p LineNumber is None.
+  /// \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, FileCheckPatternContext *Context,
+      Optional<size_t> LineNumber, const SourceMgr &SM);
   /// Parses \p Expr for a numeric substitution block. Parameter
   /// \p IsLegacyLineExpr indicates whether \p Expr should be a legacy @LINE
   /// expression. \returns a pointer to the class instance 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 being defined in this numeric
   /// substitution block, or None if this block does not define any variable.

llvm/lib/Support/FileCheck.cpp

 }
 
 char FileCheckUndefVarError::ID = 0;
 char FileCheckErrorDiagnostic::ID = 0;
 char FileCheckNotFoundError::ID = 0;
 
 Expected<FileCheckNumericVariable *>
 FileCheckPattern::parseNumericVariableDefinition(
-    StringRef &Expr, FileCheckPatternContext *Context, size_t LineNumber,
-    const SourceMgr &SM) {
+    StringRef &Expr, FileCheckPatternContext *Context,
+    Optional<size_t> LineNumber, const SourceMgr &SM) {
   Expected<VariableProperties> ParseVarResult = parseVariable(Expr, SM);
   if (!ParseVarResult)
     return ParseVarResult.takeError();
   StringRef Name = ParseVarResult->Name;
 
   if (ParseVarResult->IsPseudo)
     return FileCheckErrorDiagnostic::get(
         SM, Name, "definition of pseudo numeric variable unsupported");
if (!Expr.empty())
     return FileCheckErrorDiagnostic::get(
         SM, Expr, "unexpected characters after numeric variable name");
 
   FileCheckNumericVariable *DefinedNumericVariable;
   auto VarTableIter = Context->GlobalNumericVariableTable.find(Name);
   if (VarTableIter != Context->GlobalNumericVariableTable.end())
     DefinedNumericVariable = VarTableIter->second;
   else
-    DefinedNumericVariable = Context->makeNumericVariable(LineNumber, Name);
+    DefinedNumericVariable = Context->makeNumericVariable(Name, LineNumber);
 
   return DefinedNumericVariable;
 }
 
 Expected<std::unique_ptr<FileCheckNumericVariableUse>>
 FileCheckPattern::parseNumericVariableUse(StringRef Name, bool IsPseudo,
                                           const SourceMgr &SM) const {
   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, 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);
+    NumericVariable = Context->makeNumericVariable(Name);
     Context->GlobalNumericVariableTable[Name] = NumericVariable;
   }
 
-  if (!IsPseudo && NumericVariable->getDefLineNumber() == LineNumber)
+  Optional<size_t> DefLineNumber = NumericVariable->getDefLineNumber();
+  if (!IsPseudo && DefLineNumber && LineNumber && *DefLineNumber == *LineNumber)
     return FileCheckErrorDiagnostic::get(
         SM, Name,
         "numeric variable '" + Name + "' defined on the same line as used");
 
   return llvm::make_unique<FileCheckNumericVariableUse>(Name, NumericVariable);
 }
 
 Expected<std::unique_ptr<FileCheckExpressionAST>>
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);
+    if (LineNumber)
+      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();
FindFirstMatchingPrefix(Regex &PrefixRE, StringRef &Buffer,
 
   // 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);
+  LineVariable = makeNumericVariable(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) {
for (const auto &PatternString : Req.ImplicitCheckNot) {
     std::unique_ptr<MemoryBuffer> CmdLine = MemoryBuffer::getMemBufferCopy(
         Prefix + PatternString + Suffix, "command line");
 
     StringRef PatternInBuffer =
         CmdLine->getBuffer().substr(Prefix.size(), PatternString.size());
     SM.AddNewSourceBuffer(std::move(CmdLine), SMLoc());
 
     ImplicitNegativeChecks.push_back(
-        FileCheckPattern(Check::CheckNot, &PatternContext, 0));
+        FileCheckPattern(Check::CheckNot, &PatternContext));
     ImplicitNegativeChecks.back().parsePattern(PatternInBuffer,
                                                "IMPLICIT-CHECK", SM, Req);
   }
 
   std::vector<FileCheckPattern> DagNotMatches = ImplicitNegativeChecks;
 
   // LineNumber keeps track of the line on which CheckPrefix instances are
   // found.
if (EqIdx == StringRef::npos) {
               SM, CmdlineDef, "missing equal sign in global definition"));
       continue;
     }
 
     // Numeric variable definition.
     if (CmdlineDef[0] == '#') {
       StringRef CmdlineName = CmdlineDef.substr(1, EqIdx - 1);
       Expected<FileCheckNumericVariable *> ParseResult =
-          FileCheckPattern::parseNumericVariableDefinition(CmdlineName, this, 0,
-                                                           SM);
+          FileCheckPattern::parseNumericVariableDefinition(CmdlineName, this,
+                                                           None, 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)) {

llvm/unittests/Support/FileCheckTest.cpp

 
 static void expectUndefError(const Twine &ExpectedUndefVarName, Error Err) {
   expectUndefErrors({ExpectedUndefVarName.str()}, std::move(Err));
 }
 
 TEST_F(FileCheckTest, NumericVariable) {
   // Undefined variable: getValue and eval fail, error returned by eval holds
   // the name of the undefined variable and setValue does not trigger assert.
-  FileCheckNumericVariable FooVar = FileCheckNumericVariable(1, "FOO");
+  FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 1);
   EXPECT_EQ("FOO", FooVar.getName());
   FileCheckNumericVariableUse FooVarUse =
       FileCheckNumericVariableUse("FOO", &FooVar);
   EXPECT_FALSE(FooVar.getValue());
   Expected<uint64_t> EvalResult = FooVarUse.eval();
   EXPECT_FALSE(EvalResult);
   expectUndefError("FOO", EvalResult.takeError());
   FooVar.setValue(42);
TEST_F(FileCheckTest, NumericVariable) {
   EvalResult = FooVarUse.eval();
   EXPECT_FALSE(EvalResult);
   expectUndefError("FOO", EvalResult.takeError());
 }
 
 uint64_t doAdd(uint64_t OpL, uint64_t OpR) { return OpL + OpR; }
 
 TEST_F(FileCheckTest, Binop) {
-  FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 42);
+  FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO");
+  FooVar.setValue(42);
   std::unique_ptr<FileCheckNumericVariableUse> FooVarUse =
       llvm::make_unique<FileCheckNumericVariableUse>("FOO", &FooVar);
-  FileCheckNumericVariable BarVar = FileCheckNumericVariable("BAR", 18);
+  FileCheckNumericVariable BarVar = FileCheckNumericVariable("BAR");
+  BarVar.setValue(18);
   std::unique_ptr<FileCheckNumericVariableUse> BarVarUse =
       llvm::make_unique<FileCheckNumericVariableUse>("BAR", &BarVar);
   FileCheckASTBinop Binop =
       FileCheckASTBinop(doAdd, std::move(FooVarUse), std::move(BarVarUse));
 
   // Defined variable: eval returns right value.
   Expected<uint64_t> Value = Binop.eval();
   EXPECT_TRUE(bool(Value));
TEST_F(FileCheckTest, Substitution) {
   FileCheckStringSubstitution StringSubstitution =
       FileCheckStringSubstitution(&Context, "VAR404", 42);
   Expected<std::string> SubstValue = StringSubstitution.getResult();
   EXPECT_FALSE(bool(SubstValue));
   expectUndefError("VAR404", SubstValue.takeError());
 
   // Substitutions of defined pseudo and non-pseudo numeric variables return
   // the right value.
-  FileCheckNumericVariable LineVar = FileCheckNumericVariable("@LINE", 42);
-  FileCheckNumericVariable NVar = FileCheckNumericVariable("N", 10);
+  FileCheckNumericVariable LineVar = FileCheckNumericVariable("@LINE");
+  LineVar.setValue(42);
+  FileCheckNumericVariable NVar = FileCheckNumericVariable("N");
+  NVar.setValue(10);
   auto LineVarUse =
       llvm::make_unique<FileCheckNumericVariableUse>("@LINE", &LineVar);
   auto NVarUse = llvm::make_unique<FileCheckNumericVariableUse>("N", &NVar);
   FileCheckNumericSubstitution SubstitutionLine = FileCheckNumericSubstitution(
       &Context, "@LINE", std::move(LineVarUse), 12);
   FileCheckNumericSubstitution SubstitutionN =
       FileCheckNumericSubstitution(&Context, "N", std::move(NVarUse), 30);
   SubstValue = SubstitutionLine.getResult();