This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Report captured variables
ClosedPublic

Authored by jdenny on Jul 12 2020, 6:48 PM.

Details

Summary

Report captured variables in input dumps and traces. For example:

$ cat check
CHECK: hello [[WHAT:[a-z]+]]
CHECK: goodbye [[WHAT]]

$ FileCheck -dump-input=always -vv check < input |& tail -8
<<<<<<
           1: hello world
check:1'0     ^~~~~~~~~~~
check:1'1           ^~~~~ captured var "WHAT"
           2: goodbye world
check:2'0     ^~~~~~~~~~~~~
check:2'1                   with "WHAT" equal to "world"
>>>>>>

$ FileCheck -dump-input=never -vv check < input
check2:1:8: remark: CHECK: expected string found in input
CHECK: hello [[WHAT:[a-z]+]]
       ^
<stdin>:1:1: note: found here
hello world
^~~~~~~~~~~
<stdin>:1:7: note: captured var "WHAT"
hello world
      ^~~~~
check2:2:8: remark: CHECK: expected string found in input
CHECK: goodbye [[WHAT]]
       ^
<stdin>:2:1: note: found here
goodbye world
^~~~~~~~~~~~~
<stdin>:2:1: note: with "WHAT" equal to "world"
goodbye world
^

Diff Detail

Event Timeline

jdenny created this revision.Jul 12 2020, 6:48 PM
thopre added inline comments.Jul 13 2020, 2:09 AM
llvm/lib/Support/FileCheckImpl.h
117

I think I'd prefer this as a new field in NumericVariable. ExpressionValue is really an abstraction for signed/unsigned variable. It's used for all intermediate result of evaluating the AST of an expression and for all of those the StrValue means nothing and is of no interest. I realize the size penalty is small, it bothers me more on a conceptual level.

Does that make sense?

llvm/test/FileCheck/dump-input-annotations.txt
680

Although it will follow the same code path, I think I'd prefer a binary operation here rather than just a literal.

jdenny updated this revision to Diff 277539.Jul 13 2020, 12:35 PM

Applied reviewer suggestions. Tidied up a test some.

jdenny marked 3 inline comments as done.Jul 13 2020, 12:36 PM
jdenny added inline comments.
llvm/lib/Support/FileCheckImpl.h
117

That does make sense. Applied. Thanks!

Could you add an example where getStringValue() returns None?

llvm/lib/Support/FileCheckImpl.h
297

from it was parsed -> from which it was parsed?

jdenny updated this revision to Diff 277821.Jul 14 2020, 7:09 AM
jdenny marked 2 inline comments as done.

Address reviewer comments.

Could you add an example where getStringValue() returns None?

It's set to None for @LINE and numeric variables defined on the command line. I've extended the comments to mention these cases.

Both of these cases are tested elsewhere in the test suite. getStringValue is not called for them, so there's nothing new to test, as far as I can tell.

Does that make sense?

Could you add an example where getStringValue() returns None?

Sorry, I meant in the testcases. Maybe something with @LINE since you mention it.

Could you add an example where getStringValue() returns None?

Sorry, I meant in the testcases. Maybe something with @LINE since you mention it.

Ah, you mean the unittests, right? I keep forgetting about those for FileCheck.

jdenny updated this revision to Diff 277832.Jul 14 2020, 7:47 AM

Extend unit tests for changes to NumericVariable.

@thopre Is that what you meant? I didn't see a way to make this specific to @LINE.

Could you add an example where getStringValue() returns None?

Sorry, I meant in the testcases. Maybe something with @LINE since you mention it.

Ah, you mean the unittests, right? I keep forgetting about those for FileCheck.

Actually no, I was thinking of a case where printVariableDefs is called with a variable with no string value but I can't think of a case where this would happen. Having unittest for printVariableDefs is a good idea though.

jdenny added inline comments.Jul 14 2020, 7:49 AM
llvm/unittests/Support/FileCheckTest.cpp
684

Forgot to check getStringValue here. Will do before landing.

Extend unit tests for changes to NumericVariable.

@thopre Is that what you meant? I didn't see a way to make this specific to @LINE.

Nvm, I didn't think enough about what I said. Do you think you could also unit test printVariableDefs?

Could you add an example where getStringValue() returns None?

Sorry, I meant in the testcases. Maybe something with @LINE since you mention it.

Ah, you mean the unittests, right? I keep forgetting about those for FileCheck.

Actually no, I was thinking of a case where printVariableDefs is called with a variable with no string value but I can't think of a case where this would happen.

I haven't thought of a case either. @LINE and command-line-defined numeric variables are never defined/captured by patterns, as I understand it.

jdenny updated this revision to Diff 277852.Jul 14 2020, 9:01 AM

Removed an unused parameter from printVariableDefs.

Made unit testing of getStringValue stricter.

Added unit test for printVariableDefs. This is just a basic API test and doesn't check every aspect, such as sorting. More thorough testing appears in the FileCheck utility tests. Should we replicate all that here too? What's the general rule?

jdenny marked an inline comment as done.Jul 14 2020, 9:02 AM
thopre accepted this revision.Jul 14 2020, 1:06 PM

Removed an unused parameter from printVariableDefs.

Made unit testing of getStringValue stricter.

Added unit test for printVariableDefs. This is just a basic API test and doesn't check every aspect, such as sorting. More thorough testing appears in the FileCheck utility tests. Should we replicate all that here too? What's the general rule?

Unfortunately there's no rule right now and there's already overlap. My personal feeling is:

  • Every new code should have unit test with a focus on code coverage
  • User visible behavior of a code should be tested in llvm/test (it does not need to aim at full coverage since there is unittest)
  • Try to avoid duplication as much as possible
  • Aim for readability

In this specific example the test in llvm/test shows the definition diagnostic alongside other diagnostic. The unit test would cover all possible case (e.g. loops taken/not taken). Unfortunately this already leads to some duplication so perhaps keep the sorting for the llvm/test which is much easier to read in llvm/test since output with several variable definition spans several lines.

I think the general rule should be discussed separately and I'm happy to approve the patch as it is.

This revision is now accepted and ready to land.Jul 14 2020, 1:06 PM

I think the general rule should be discussed separately

Agreed. Thanks for presenting your view.

and I'm happy to approve the patch as it is.

Thanks for the review. This cannot land without D83650, so we have some time still to extend the testing if needed.

This revision was automatically updated to reflect the committed changes.