[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 marked an inline comment as done.
jdenny added a comment.

What test suites did you try this on?  It's good to be cautious when increasing FileCheck's strictness.



================
Comment at: llvm/lib/Support/FileCheck.cpp:1375-1376
+  // and use the first prefix as a filler for the error message.
+  // We do not allow using -implicit-check-not when an explicitly specified
+  // check prefix is not present in the input buffer.
+  bool IsDefaultPrefix =
----------------
grimar wrote:
> jdenny wrote:
> > What if the only explicitly specified prefix is `CHECK`?  Oddly, it happens.
> Yeah, I thought about this strange case. 
> In the implementation of the first diff, such case could produce a false-positive result when
> `CHECK` lines are missing.
> 
> I was not sure how we might want to handle it. I prefer to not use `--check-prefix=CHECK` alone
> as it probably just adds noise to the test cases. `FileCheck` perhaps could report a error to ban this case.
> 
> Though thinking about it again, there can be other cases where `CHECK` is involved,
> perhaps the simplest/cleanest way to go is just to recognize the default case better and handle it too?
> I've added a flag to `FileCheckRequest` to implement it.
> (That is was what I was not sure is worth doing - adding a new flag).
> 
> 
> 
> 
I think that's a good solution.  Thanks.


================
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()) {
----------------
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(
  . . .
}
```


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

https://reviews.llvm.org/D78024