[PATCH] D64228: [FileCheck] Don't diagnose undef vars at parse time

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:160
+  // of this variable use. If we don't find a variable definition we create a
+  // dummy one so that parsing can continue. All use of undefined variables,
+  // whether string or numeric, are then diagnosed when printSubstitutions()
----------------
use -> uses


================
Comment at: llvm/lib/Support/FileCheck.cpp:161
+  // dummy one so that parsing can continue. All use of undefined variables,
+  // whether string or numeric, are then diagnosed when printSubstitutions()
+  // after failing to match.
----------------
when -> during (?)


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:72
 ; Numeric expression using undefined variable.
-RUN: not FileCheck --check-prefix UNDEF-USE --input-file %s %s 2>&1 \
-RUN:   | FileCheck --strict-whitespace --check-prefix UNDEF-USE-MSG %s
+RUN: not FileCheck --check-prefix UNDEF-USE --input-file %s %s
 
----------------
Have you lost test coverage here? It looks to me like you're no longer checking the error message printed at all, and I don't understand why from your comment.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:474
                                                DefinedNumericVariable, SM);
-  EXPECT_TRUE(errorToBool(Expression.takeError()));
+  EXPECT_TRUE(static_cast<bool>(Expression));
+  ExpressionVal = (*Expression)->eval();
----------------
If EXPECT_TRUE doesn't work with Expression, can you call the boolean operator of Expression here? I think that would be nicer than a static_cast.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:480
+  // Clear again because parseNumericSubstitutionBlock would have created a
+  // dummy variable and store it in GlobalNumericVariableTable.
+  Cxt.clearLocalVars();
----------------
store -> stored


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64228/new/

https://reviews.llvm.org/D64228