Page MenuHomePhabricator

[FileCheck] Fix numeric variable redefinition
ClosedPublic

Authored by thopre on Jul 17 2019, 2:00 PM.

Details

Summary

Commit r365249 changed usage of FileCheckNumericVariable to have one
instance of that class per variable as opposed to one instance per
definition of a given variable as was done before. However, it retained
the safety check in setValue that it should only be called with the
variable unset, even after r365625.

However this causes assert failure when a non-pseudo variable is being
redefined. And while redefinition of @LINE at each CHECK line work in
the general case, it caused problem when a substitution failed (fixed in
r365624) and still causes problem when a CHECK line does not match since
@LINE's value is cleared after substitutions in match() happened but
printSubstitutions also attempts a substitution.

This commit solves the root of the problem by changing setValue to set a
new value regardless of whether a value was set or not, thus fixing all
the aforementioned issues.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.Jul 17 2019, 2:00 PM

I am wondering why we don't just have setValue call clearValue first in all cases?

I am wondering why we don't just have setValue call clearValue first in all cases?

That's a remnant when each variable definition had its own variable and thus setting it twice didn't make sense and indicated an error. I think the right thing to do now is to change setValue to not check if the value is already set or not.

I am wondering why we don't just have setValue call clearValue first in all cases?

That's a remnant when each variable definition had its own variable and thus setting it twice didn't make sense and indicated an error. I think the right thing to do now is to change setValue to not check if the value is already set or not.

That seems reasonable to me. I think that would make this patch largely redundant, right?

thopre updated this revision to Diff 210516.Jul 18 2019, 3:07 AM

Pursue different approach

thopre retitled this revision from [FileCheck] Fix @LINE substitution in error msg to [FileCheck] Fix numeric variable redefinition.Jul 18 2019, 3:08 AM
thopre edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jul 18 2019, 3:23 AM

LGTM.

I think I'll add unit tests as well.

LGTM.

I think I'll add unit tests as well.

Nvm, one issue is an assert and the other needs to test the diagnostic message, both aspects being difficult to test in unit testing. There's already tests for those so I'll leave it like that.

This revision was automatically updated to reflect the committed changes.