[PATCH] D54769: [FileCheck] New option -warn

Mon Dec 3 12:13:26 PST 2018

jdenny added a comment.

In D54769#1317118 <https://reviews.llvm.org/D54769#1317118>, @dblaikie wrote:

> In D54769#1317107 <https://reviews.llvm.org/D54769#1317107>, @jdenny wrote:
>
> > For simple tests, an external script could parse (static) test files as I described.  That script could run as a separate test case.
>
>
> Not exactly possible, unfortunately - tests can/should only read files in the "Inputs" directory relative to the test file location - they can't walk the entire test tree/read everything (this keeps tests a bit isolated, so you can move them around without worrying about some other random test depending on your test - and also it's used internally at Google to avoid shipping all the tests/inputs to every machine that runs any of the tests)

For something that checks correctness of test suites (and assuming we decide it's worth it), couldn't we ultimately change the rules?  It might even be a lit feature rather than a distinct test.  It could also be optional in case it's problematic somewhere.

>> I'm not sure which tests would be "simple", but any test that generates the FileCheck check file dynamically would certainly not work with such a script.
> 
> I don't think there are any of those - at least I rather hope not.

I didn't find any with a quick grep, but I feel like I didn't look hard enough.  I know I have some in phabricator for FileCheck's own test suite, but reviewers might tell me to remove that usage.

Another problem would be if lit substitutions make FileCheck commands or their prefixes unrecognizable.  I haven't looked for that usage yet.

> 
> 
>> For a (hopefully) complete solution, here's a possible approach.  Each FileCheck call builds (1) a list of all prefixes defined (by `--check-prefix[es]`) for that one call and (2) a list of all prefixes used in directives but not defined in the first list, and then it merges those lists into a database.  At the end of a test suite run, lit calls a special FileCheck incantation that compiles that database into a list of unused directives.
> 
> "merges those lists into a database" is a bit vague/concerning in terms of how that'd run in the build system, etc.

Sure, I was just trying to communicate the high-level idea.  I'll fill in some details, but these are just possibilities.

For 2, we'd probably want a list of all //directives// whose prefixes are not defined in 1.

Each entry in 1 and 2 would specify what FileCheck check file it was associated with (and that might present a challenge if that file is ever a stream).

The "merging" that each FileCheck calls performs might be just appending its lists to a results file that is unique to the lit test file that contains that FileCheck call.  Hopefully this would permit merging to be performed without synchronization across parallel tests.

For the final compilation of the database, the procedure would be to iterate all these result files.  Per FileCheck check file, it would first build a hash of all defined prefixes, and then it would build a list of all unused directives but removing any whose prefix appears in the hash.

The final compilation of the database could be a feature of lit that is always run at the end of any test suite run.

> 
> 
>>> Each FileCheck run would itself look for other FileCheck commands in RUN lines to compute the total set of prefixes used & validate them all? (that'd mean redundant work (O(N^2) in the number of FileCheck commands/prefixes - but probably scale OK) & redundant error messages (since each FileCheck command would report all the unused check names itself))
>> 
>> That seems possible too.  We could experiment with different approaches.
> 
> Yeah, I'd suggest scratching something simple up with grep/sed/python/whatever to get a sense of the magnitude of the problem before sinking time into trying to design/build a long-term solution. My feeling is still that this isn't a problem worth solving given the complexity of the solutions thus far (the self-checking FileCheck version (despite its duplicate error messages, etc) feels like the cheapest I can think of & still isn't terribly nice, I think) - but data will help.

Agreed.  No promises I'll do this anytime soon.  I just want to see if people feel that, if this mistyped/unused directive problem is actually worth addressing, this approach is the right one.

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

https://reviews.llvm.org/D54769