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

[George Rimar via Phabricator via cfe-commits]

grimar marked an inline comment as done.
grimar 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()) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78024