[FileCheck] Factor some parsing checks out

[thopre]

Summary

Both callers of parseNumericVariableDefinition() perform the same extra
check that no character is found after the variable name. This patch
factors out this check into parseNumericVariableDefinition().

[jhenderson]

LGTM, assuming the error position difference is not an issue.

llvm/lib/Support/FileCheck.cpp

251 ↗

(On Diff #208093)

Nit: is this needed here?

llvm/test/FileCheck/numeric-defines-diagnostics.txt

23 ↗

(On Diff #208093)

It's slightly disconcerting that the column number here has changed. I assume that it's just some difference between the start and end of the string or something?

[arichardson]

llvm/test/FileCheck/numeric-defines-diagnostics.txt

23 ↗

(On Diff #208093)

It seems like the diagnostic now points to the invalid character (+) instead of the start of the name. I think this makes more sense.

[thopre]

Address review comments

[thopre]

llvm/test/FileCheck/numeric-defines-diagnostics.txt

23 ↗

(On Diff #208093)

As Alexander said, it was pointing to the start of the variable before (saying basically that the whole "VALUE+2" is an invalid variable name) and is now pointing to the + operator because that's the first unexpected character.

llvm/trunk/lib/Support/FileCheck.cpp

Error FileCheckPattern::parseNumericVariableDefinition(
 
   // 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();
 }
 
 Expected<FileCheckNumericVariable *>
 FileCheckPattern::parseNumericVariableUse(StringRef &Expr,
                                           const SourceMgr &SM) const {
   bool IsPseudo;
   Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
Expected<FileCheckExpression *> FileCheckPattern::parseNumericSubstitutionBlock(
     StringRef Expr,
     Optional<FileCheckNumericVariable *> &DefinedNumericVariable,
     const SourceMgr &SM) const {
   // Parse the numeric variable definition.
   DefinedNumericVariable = None;
   size_t DefEnd = Expr.find(':');
   if (DefEnd != StringRef::npos) {
     StringRef DefExpr = Expr.substr(0, DefEnd);
-    StringRef UseExpr = Expr = Expr.substr(DefEnd + 1);
-
-    DefExpr = DefExpr.ltrim(SpaceChars);
-    StringRef Name;
-    Error ErrorDiagnostic =
-        parseNumericVariableDefinition(DefExpr, Name, Context, SM);
-    if (ErrorDiagnostic)
-      return std::move(ErrorDiagnostic);
+    StringRef UseExpr = Expr.substr(DefEnd + 1);
 
-    DefinedNumericVariable =
-        Context->makeNumericVariable(this->LineNumber, Name);
-
-    DefExpr = DefExpr.ltrim(SpaceChars);
-    if (!DefExpr.empty())
-      return FileCheckErrorDiagnostic::get(
-          SM, DefExpr, "invalid numeric variable definition");
     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);
+
     return Context->makeExpression(add, nullptr, 0);
   }
 
   // Parse the expression itself.
   Expr = Expr.ltrim(SpaceChars);
   return parseBinop(Expr, SM);
 }
 
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);
       StringRef VarName;
-      SMLoc CmdlineNameLoc = SMLoc::getFromPointer(CmdlineName.data());
       Error ErrorDiagnostic = FileCheckPattern::parseNumericVariableDefinition(
           CmdlineName, VarName, this, SM);
       if (ErrorDiagnostic) {
         Errs = joinErrors(std::move(Errs), std::move(ErrorDiagnostic));
         continue;
       }
-      // Check that CmdlineName is only composed of the parsed numeric
-      // variable. This catches cases like "FOO+2" in a "FOO+2=10" definition.
-      if (!CmdlineName.empty()) {
-        Errs = joinErrors(std::move(Errs),
-                          FileCheckErrorDiagnostic::get(
-                              SM, CmdlineNameLoc, "invalid variable name"));
-        continue;
-      }
-
-      // Detect collisions between string and numeric variables when the latter
-      // is created later than the former.
-      if (DefinedVariableTable.find(VarName) != DefinedVariableTable.end()) {
-        Errs = joinErrors(
-            std::move(Errs),
-            FileCheckErrorDiagnostic::get(SM, VarName,
-                                          "string variable with name '" +
-                                              VarName + "' already exists"));
-        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 '" +

llvm/trunk/test/FileCheck/numeric-defines-diagnostics.txt

 NUMERRCLIPSEUDO: Global defines:1:20: error: definition of pseudo numeric variable unsupported
 NUMERRCLIPSEUDO-NEXT: Global define #1: #@VALUE=10
 NUMERRCLIPSEUDO-NEXT: {{^                   \^$}}
 
 ; Invalid definition of an expression.
 RUN: not FileCheck -D#VALUE+2=10 --input-file %s %s 2>&1 \
 RUN:   | FileCheck %s --strict-whitespace --check-prefix NUMERRCLITRAIL
 
-NUMERRCLITRAIL: Global defines:1:20: error: invalid variable name
+NUMERRCLITRAIL: Global defines:1:25: error: unexpected characters after numeric variable name
 NUMERRCLITRAIL-NEXT: Global define #1: #VALUE+2=10
-NUMERRCLITRAIL-NEXT: {{^                   \^$}}
+NUMERRCLITRAIL-NEXT: {{^                        \^$}}
 
 ; Invalid value: numeric expression instead of literal.
 RUN: not FileCheck -D#VALUE1=3 -D#VALUE2='VALUE1 + 2' --input-file %s %s 2>&1 \
 RUN:   | FileCheck %s --strict-whitespace --check-prefix NUMERRCLIEXPR
 
 NUMERRCLIEXPR: Global defines:2:27: error: invalid value in numeric variable definition 'VALUE1 + 2'
 NUMERRCLIEXPR-NEXT: Global define #2: #VALUE2=VALUE1 + 2
 NUMERRCLIEXPR-NEXT: {{^                          \^$}}