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

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

jdenny accepted this revision.
jdenny added a comment.
This revision is now accepted and ready to land.

In D78024#1983304 <https://reviews.llvm.org/D78024#1983304>, @grimar wrote:

> In D78024#1981409 <https://reviews.llvm.org/D78024#1981409>, @jdenny wrote:
>
> > What test suites did you try this on?  It's good to be cautious when increasing FileCheck's strictness.
>
>
> I used run check-llvm and check-lld previously for this patch.
>  For the latest diff I've used check-all (which revealed one more test case failture in CodeGen/catch-implicit-conversions-basics-negatives.c)..


In case you didn't configure all LLVM projects or some tests were skipped for your config, please watch the bots carefully.

LGTM except for some nits on the comments.

Thanks for this!



================
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:
> > 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.
```



================
Comment at: llvm/lib/Support/FileCheck.cpp:1393-1394
 
+  // Add an EOF pattern for any trailing CHECK-DAG/-NOTs, and use the first
+  // prefix as a filler for the error message.
+  if (!DagNotMatches.empty()) {
----------------
Your original patch mentioned -implicit-check-not here.  I thought that was helpful.  Is there a reason to drop it?


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

https://reviews.llvm.org/D78024