[PATCH] D64228: [FileCheck] Don't diagnose undef vars at parse time
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 5 01:32:52 PDT 2019
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
More information about the llvm-commits
mailing list