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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

I've been thinking about these diagnostics over the past week.  I'm afraid I'm becoming convinced they are not the best approach.  As evidenced by the false positives we've already discussed, I believe unused prefixes and the absence of non-label directives are not inherently bugs.  Instead, we are using them as heuristics to help us find the real bugs: //typos// in FileCheck directives that result in //unused directives//.

Problem: I believe we are resorting to such heuristics because of a flaw in FileCheck's //general// directive syntax.  Looking at the function `ValidateCheckPrefixes` in `lib/Support/FileCheck.cpp`, that syntax is just a word (`[a-zA-Z0-9_-]+`) followed by a colon.  Without knowing the list of user-defined prefixes in order to restrict the syntax further, there is no reliable way to distinguish FileCheck directives from other text in a FileCheck check file.  Thus, there is no way to automatically detect directives that are never used because their prefixes are never defined by the user.

Solution: I think we should design a general directive syntax that is sufficiently //distinct//.  As I have been reminded many times before, tools like FileCheck are intended for LLVM's internal use not for the general public.  In other words, FileCheck should do what LLVM developers need not the other way around.  With this in mind, we should carefully design a general syntax that is very unlikely to collide with the kinds of non-directive text in LLVM's //own// test suites and not worry too much about all conceivable test suites in the wild.  If we are clever in our choice, my hunch is that such collisions will rarely occur, and it will be possible to work around cases where they do.  After all, even now, how often do we have to choose a prefix other than `CHECK` purely because of such collisions?  We normally change prefixes just to activate a different set of directives, right?  We just need to add something that distinctive into the //fixed// portion of the general syntax.  (To mock something up at first, we could just pick a small set of tests and change all their user-defined prefixes to start with the same distinctive string.  For example, we might have `FC-CHECK`, `FC-FOO`, `FC-BAR`, etc., but maybe there's something better.)

Conclusion: A distinct general syntax for FileCheck directives would permit us to implement //precise// detection of //all// unused directives and thus more cleanly combat directive typos.  There's plenty to be discussed, and the migration to a new syntax would be a long-term investment (but it can be done incrementally).  If the problem with typos is as bad as evidence seems to suggest so far, perhaps all that effort will prove worthwhile: it tackles the root of our problem rather than placing awkward band-aids over it as we're trying to do now.  What do people think?


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

https://reviews.llvm.org/D54769