Page MenuHomePhabricator

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

Authored by thopre on Jul 4 2019, 4:49 PM.



Diagnosing use of undefined variables takes place in
parseNumericVariableUse() and printSubstitutions() for numeric variables
but only takes place in printSubstitutions() for string variables. The
reason for the split location of diagnostics is that parsing is not
aware of the clearing of variables due to --enable-var-scope and thus
use of variables cleared in this way can only be catched by

Beyond the code level inconsistency, there is also a user facing
inconsistency since diagnostics look different between the two
functions. While the diagnostic in printSubstitutions is more verbose,
doing the diagnostic there allows to diagnose all undefined variables
rather than just the first one and error out.

This patch create dummy variable definition when encountering a use of
undefined variable so that parsing can proceed and be diagnosed by
printSubstitutions() later. Tests that were testing whether parsing
fails in such case are thus modified accordingly.

Diff Detail


Event Timeline

thopre created this revision.Jul 4 2019, 4:49 PM
jhenderson added inline comments.Jul 5 2019, 1:32 AM
160 ↗(On Diff #208095)

use -> uses

161 ↗(On Diff #208095)

when -> during (?)

72 ↗(On Diff #208095)

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.

474 ↗(On Diff #208095)

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.

480 ↗(On Diff #208095)

store -> stored

thopre updated this revision to Diff 208141.Jul 5 2019, 4:28 AM
thopre marked 5 inline comments as done.

Address review comments

thopre marked an inline comment as done.Jul 5 2019, 4:28 AM
thopre added inline comments.
72 ↗(On Diff #208095)

I checked llvm/test/FileCheck/verbose.txt and thought it was already tested there but looking at it again today it's clearly not the case. Shouldn't have submitted those patches so late. The codepath is tested via llvm/test/FileCheck/var-scope.txt but (1) not the full diagnostic is checked for (only the error message) and (2) this only checks undefined variable due to --enable-var-scope.

I've recycled the test for undefined numeric variables, I'll submit a separate patch later to add a test for undefined string variables.

This revision is now accepted and ready to land.Jul 5 2019, 4:34 AM
thopre updated this revision to Diff 208147.Jul 5 2019, 4:51 AM
thopre marked an inline comment as done.

Use relative line number in test

jhenderson accepted this revision.Jul 5 2019, 5:37 AM
This revision was automatically updated to reflect the committed changes.