[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

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

LGTM, but might not hurt to have someone else looking at it.



================
Comment at: llvm/test/FileCheck/comment/bad-comment-prefix.txt:40
+RUN:                                      -check-prefixes=RUN,FOO | \
+RUN:   FileCheck -check-prefix=CHECK-PREFIX-DUP-RUN_ %s
+CHECK-PREFIX-DUP-COM: error: supplied check prefix must be unique among check and comment prefixes: 'COM'
----------------
I'm guessing the underscore is to circumvent lit trying to run things? If so, I think we need to fix lit to only run lines where the RUN: is at the start of the line (ignoring whitespace and non alpha-numerics). I don't think anything else makes sense.

Not related to this patch of course, so nothing to do here, unless my guess is incorrect.


================
Comment at: llvm/test/FileCheck/comment/suffixes.txt:1-2
+# Comment prefixes plus check directive suffixes are not comment directives
+# and are treated as plain text.
+
----------------
jdenny wrote:
> jhenderson wrote:
> > I don't think you should change anything here, but if I'm following this right, this leads to the amusing limitation that you can "comment out" (via --comment-prefixes) any CHECK lines that use a suffix (CHECK-NEXT, CHECK-NOT etc) but not those that don't without changing --check-prefixes value!
> > 
> > By the way, do we need to have a test-case for that? I.e. that --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming it does of course)?
> > I don't think you should change anything here, but if I'm following this right, this leads to the amusing limitation that you can "comment out" (via --comment-prefixes) any CHECK lines that use a suffix (CHECK-NEXT, CHECK-NOT etc) but not those that don't without changing --check-prefixes value!
> 
> That's right, but check prefixes have this problem too.  That is, you can do things like `-check-prefixes=FOO,FOO-NOT` so that `FOO-NOT` is not negative.
> 
> `ValidatePrefixes` should be extended to catch such cases, I think.  But in a separate patch.
> 
> > By the way, do we need to have a test-case for that? I.e. that --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming it does of course)?
> 
> Hmm.  I think it's behavior we don't want to support.  Maybe the test case should be added when extending `ValidatePrefixes` as I described above?
> 
I agree it's separate work. FWIW, I just came up with a genuinely useful use-case for it with CHECK directives, but it might just be unnecessary. Imagine the case where you want a test where some specific output is printed under one condition and not another condition. You'd want something like:

```
# RUN: ... | FileCheck %s --check-prefix=WITH
# RUN: ... | FileCheck %s --check-prefix=WITHOUT

# WITH: some text that should be matched
# WITHOUT-NOT: some text that should be matched
```

A careleses developer could change the text of "WITH" to match some new behaviour without changing "WITHOUT-NOT", thus breaking the second case. You could instead do:

```
# RUN: ... | FileCheck %s --check-prefix=CHECK-NOT
# RUN: ... | FileCheck %s

# CHECK-NOT: some text that should be matched
```
Then, if the output changed, you'd update both the regular and NOT match. I might have used this pattern in a few tests in the past had it occurred to me.

Anyway, I think there are other ways of solving that problem, although not without work on FileCheck (I've been prototyping a method with only limited success so far), and I agree it's otherwise mostly horrible, so I'm not seriously opposing the suggestion.


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

https://reviews.llvm.org/D79276