[FileCheck] Record numeric variable availability

[thopre]

Summary

Change the line number information stored for a numeric variable to be
about when does the value becomes available rather than when it is
defined. This provides a more natural semantic for pseudo variables and
variables defined on the command-line that are not defined from a
variable definition in the input text.

[jhenderson]

As mentioned inline, I don't understand what you mean by "available". Perhaps you could post some basic examples for the different cases explaining what you mean, and then I can try suggesting some different terminology.

llvm/include/llvm/Support/FileCheck.h

52–53

I'm not sure it's clear to me what "available" means in this context if it doesn't mean definition.

56–66

in input -> in the input (?)

58

Not that I'm bothered really, but is there a reason to change the signature order here?

[thopre]

Rebase on top of latest changes in patch series

[thopre]

llvm/include/llvm/Support/FileCheck.h

52–53

The change in naming is mostly for pseudo variables (@LINE being the only at the moment) where the definition is implicit and thus there isn't a definition line number per se. However I think it makes sense to talk about value availability (e.g. @LINE only has a value from the first line in the CHECK file, whereas other variable have their value available from after their definition).

58

1. Consistency
2. I usually put the most important parameters first and the name of a variable feels more essential (since that's how you refer to it). The line number is only to catch some incorrect uses

[jhenderson]

llvm/include/llvm/Support/FileCheck.h

52–53

I could spin it another way. The @LINE variable is defined anew on each line, with the value of the input line number. Alternatively, it is defined on the first line, and updated thereafter. Thus its definition line is equal to its current value, or the first time it was defined (i.e. line 1), whichever is more useful.

Thinking in C++ terms, there is no concept of variable "availability". You have scope, definition, and declaration as related words. Thus variable availability is not a concept that people necessarily will understand.

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's value becomes available.
+  size_t AvailLineNumber;
 
 public:
-  /// Constructor for a variable \p Name defined at line \p DefLineNumber.
-  FileCheckNumericVariable(size_t DefLineNumber, StringRef Name)
-      : 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) {}
+  /// Constructor for a numeric variable \p Name defined from a value in the
+  /// input and whose value becomes available at line \p AvailLineNumber.
+  FileCheckNumericVariable(StringRef Name, size_t AvailLineNumber)
+      : Name(Name), AvailLineNumber(AvailLineNumber) {}
+
+  /// Constructor for a numeric variable \p Name whose \p Value is known at
+  /// parse time and becomes available at line \p AvailLineNumber (e.g. the
+  /// @LINE numeric variable).
+  FileCheckNumericVariable(StringRef Name, uint64_t Value,
+                           size_t AvailLineNumber)
+      : Name(Name), Value(Value), AvailLineNumber(AvailLineNumber) {}
 
   /// \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 not defined. \returns whether the
   /// variable was already defined.
   bool setValue(uint64_t Value);
 
   /// Clears value of this numeric variable. \returns whether the variable was
   /// already undefined.
   bool clearValue();
 
-  /// \returns the line number where this variable is defined.
-  size_t getDefLineNumber() { return DefLineNumber; }
+  /// \returns the line number where this variable's value becomes available.
+  size_t getAvailLineNumber() { return AvailLineNumber; }
 };
 
 /// Type of functions evaluating a given binary operation.
 using binop_eval_t = uint64_t (*)(uint64_t, uint64_t);
 
 /// Class to represent an undefined variable error which prints that variable's
 /// name between quotes when printed.
 class FileCheckUndefVarError : public ErrorInfo<FileCheckUndefVarError> {
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 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.
+  /// Parses \p Expr for the name of a numeric variable whose value becomes
+  /// available at line \p AvailLineNumber. \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);
+                                 size_t AvailLineNumber, 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

 }
 
 char FileCheckUndefVarError::ID = 0;
 char FileCheckErrorDiagnostic::ID = 0;
 char FileCheckNotFoundError::ID = 0;
 
 Expected<FileCheckNumericVariable *>
 FileCheckPattern::parseNumericVariableDefinition(
-    StringRef &Expr, FileCheckPatternContext *Context, size_t LineNumber,
+    StringRef &Expr, FileCheckPatternContext *Context, size_t AvailLineNumber,
     const SourceMgr &SM) {
   bool IsPseudo;
   Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
   if (!ParseVarResult)
     return ParseVarResult.takeError();
   StringRef Name = *ParseVarResult;
 
   if (IsPseudo)
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, AvailLineNumber);
 
   return DefinedNumericVariable;
 }
 
 Expected<FileCheckNumericVariable *>
 FileCheckPattern::parseNumericVariableUse(StringRef &Expr,
                                           const SourceMgr &SM) const {
   bool IsPseudo;
FileCheckPattern::parseNumericVariableUse(StringRef &Expr,
   // 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, 0);
     Context->GlobalNumericVariableTable[Name] = NumericVariable;
   }
 
-  if (!IsPseudo && NumericVariable->getDefLineNumber() == LineNumber)
+  if (!IsPseudo && NumericVariable->getAvailLineNumber() > LineNumber)
     return FileCheckErrorDiagnostic::get(
         SM, Name,
         "numeric variable '" + Name + "' defined on the same line as used");
 
   return NumericVariable;
 }
 
 static uint64_t add(uint64_t LeftOp, uint64_t RightOp) {
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);
     Expected<FileCheckNumericVariable *> ParseResult =
-        parseNumericVariableDefinition(DefExpr, Context, LineNumber, SM);
+        parseNumericVariableDefinition(DefExpr, Context, LineNumber + 1, SM);
     if (!ParseResult)
       return ParseResult.takeError();
     DefinedNumericVariable = *ParseResult;
 
     return Context->makeExpression(add, nullptr, 0);
   }
 
   // Parse the expression itself.
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, 1);
   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) {
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,
+          FileCheckPattern::parseNumericVariableDefinition(CmdlineName, this, 1,
                                                            SM);
       if (!ParseResult) {
         Errs = joinErrors(std::move(Errs), ParseResult.takeError());
         continue;
       }
 
       StringRef CmdlineVal = CmdlineDef.substr(EqIdx + 1);
       uint64_t Val;

llvm/unittests/Support/FileCheckTest.cpp

 
 using namespace llvm;
 namespace {
 
 class FileCheckTest : public ::testing::Test {};
 
 TEST_F(FileCheckTest, NumericVariable) {
   // Undefined variable: getValue and clearValue fails, setValue works.
-  FileCheckNumericVariable FooVar = FileCheckNumericVariable(1, "FOO");
+  FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 1);
   EXPECT_EQ("FOO", FooVar.getName());
   llvm::Optional<uint64_t> Value = FooVar.getValue();
   EXPECT_FALSE(Value);
   EXPECT_TRUE(FooVar.clearValue());
   EXPECT_FALSE(FooVar.setValue(42));
 
   // Defined variable: getValue returns value set, setValue fails.
   Value = FooVar.getValue();

 
 static void expectUndefError(const Twine &ExpectedStr, Error Err) {
   handleAllErrors(std::move(Err), [&](const FileCheckUndefVarError &E) {
     EXPECT_EQ(ExpectedStr.str(), E.getVarName());
   });
 }
 
 TEST_F(FileCheckTest, Expression) {
-  FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 42);
+  FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 42, 1);
   FileCheckExpression Expression = FileCheckExpression(doAdd, &FooVar, 18);
 
   // Defined variable: eval returns right value.
   Expected<uint64_t> Value = Expression.eval();
   EXPECT_TRUE(static_cast<bool>(Value));
   EXPECT_EQ(60U, *Value);
 
   // Undefined variable: eval fails, undefined variable returned. We call
TEST_F(FileCheckTest, Substitution) {
   FileCheckStringSubstitution StringSubstitution =
       FileCheckStringSubstitution(&Context, "VAR404", 42);
   Expected<std::string> SubstValue = StringSubstitution.getResult();
   EXPECT_FALSE(static_cast<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", 42, 1);
+  FileCheckNumericVariable NVar = FileCheckNumericVariable("N", 10, 1);
   FileCheckExpression LineExpression = FileCheckExpression(doAdd, &LineVar, 0);
   FileCheckExpression NExpression = FileCheckExpression(doAdd, &NVar, 3);
   FileCheckNumericSubstitution SubstitutionLine =
       FileCheckNumericSubstitution(&Context, "@LINE", &LineExpression, 12);
   FileCheckNumericSubstitution SubstitutionN =
       FileCheckNumericSubstitution(&Context, "N", &NExpression, 30);
   Expected<std::string> Value = SubstitutionLine.getResult();
   EXPECT_TRUE(static_cast<bool>(Value));