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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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 =
----------------
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).






================
Comment at: llvm/test/FileCheck/implicit-check-not.txt:3-11
+; Check we report an error when an unknown prefix is used together with `-implicit-check-not`
+; RUN: sed 's#^;.*##' %s | not FileCheck -check-prefix=UNKNOWN-PREFIX -implicit-check-not=abc %s 2>&1 | FileCheck %s -check-prefix CHECK-PREFIX-ERROR
+; CHECK-PREFIX-ERROR: error: no check strings found with prefix 'UNKNOWN-PREFIX:'
+
+; Check we allow using `-implicit-check-not` when there is no `-check-prefix` specified and there
+; is no default `CHECK` line in an input. Use an arbitrary random unique string as an
+; argument for `-implicit-check-not`.
----------------
jdenny wrote:
> All of these new tests pass for me even without your changes to FileCheck.
Ah, I am sorry. For the first test case it happened because of the following check line:
`error: no check strings found with prefix 'UNKNOWN-PREFIX:'`
which reported the unknown prefix in its body (and so `FoundUsedPrefix` was set to `true` because of that.
It just a bit unusual to me that a check line can trigger the different test case behavior..).

The second test is new in the latest diff.

And the third test is expected to pass with or without this patch, I've added it because it is related
to changes done by this patch and there seems to be no test case anywhere which checks the same.


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

https://reviews.llvm.org/D78024