[PATCH] D78024: [FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix.

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

jdenny marked an inline comment as done.
jdenny added a subscriber: SjoerdMeijer.
jdenny added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1375-1377
+  // We do not allow using -implicit-check-not when an explicitly specified
+  // check prefix is not present in the input buffer.
+  if ((Req.IsDefaultCheckPrefix || FoundUsedPrefix) && !DagNotMatches.empty()) {
----------------
grimar wrote:
> jdenny wrote:
> > grimar wrote:
> > > jdenny wrote:
> > > > I find this logic confusing.  Its goal appears to be to constrain when `DagNotMatches` are added to `CheckStrings`.  However, the real goal is to constrain when FileCheck complains that there are no used prefixes.  Would the following logic work?
> > > > 
> > > > ```
> > > > if (!FoundUsedPrefix && (ImplicitNegativeChecks.empty() || !Req.IsDefaultCheckPrefix)) {
> > > >   errs() << "error: no check strings found with prefix"
> > > >   . . .
> > > > }
> > > > if (!DagNotMatches.empty()) {
> > > >   CheckStrings->emplace_back(
> > > >   . . .
> > > > }
> > > > ```
> > > This looks better and works, thanks!
> > > 
> > > (The code above has `DagNotMatches = ImplicitNegativeChecks;` line which is also a bit confusing probably)
> > > This looks better and works, thanks!
> > 
> > Thanks for making that change.
> > 
> > > (The code above has DagNotMatches = ImplicitNegativeChecks; line which is also a bit confusing probably)
> > 
> > I'm fine with that one.  DagNotMatches starts out as ImplicitNegativeChecks, and CHECK-NOTs might be added later.
> > 
> > I think the second sentence in the following comment still reflects the old logic:
> > 
> > ```
> >   // When there are no used prefixes we report an error.
> >   // We do not allow using -implicit-check-not when an explicitly specified
> >   // check prefix is not present in the input buffer.
> > ```
> > 
> > Something like the following seems better to me:
> > 
> > ```
> >   // When there are no used prefixes we report an error except in the case that
> >   // no prefix is specified explicitly but -implicit-check-not is specified.
> > ```
> > 
> > the real goal is to constrain when FileCheck complains that there are no used prefixes
> 
> btw, do you know why FileCheck seems intentionally allows the case when `--check-prefixes=KNOWN,UNKNOWN`?
> I've mentioned in the description of D78110 that there are about 1000 tests that have this. but is it a feature or a something that we might want to fix?
There was a long discussion about diagnostics like that over a year ago.  I believe we described that case as prefixes that are defined but not used.

In some of my downstream work, I often create FileCheck prefix schemes reflecting various combinations of features I want to test.  For example: FOO, BAR, FOO-AND-BAR, FOO-NOT-BAR, etc.  It's far easier to maintain those tests if I list all appropriate prefixes for each FileCheck command even if some prefixes are not currently used in the test file.  I don't know if this use case exists upstream right now.

Also, when developing or debugging tests involving many prefixes and many FileCheck commands, I think it could be painful to have to update all FileCheck commands every time you temporarily remove the only uses of some prefix.

If you pursue this diagnostic, please make it optional, even if on by default.  At one time, we discussed a warning system for such diagnostics.  Warnings could be configurable via command-line options, possibly passed via the FILECHECK_OPTS env var so that it's easy to relax them during debugging or based on a test suite's specific needs.

@probinson and @SjoerdMeijer might want to chime in as they were also part of previous discussions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78024