[PATCH] D79276: [FileCheck] Support comment directives

Thu May 7 07:54:46 PDT 2020

jdenny marked 5 inline comments as done.
jdenny added a comment.

In D79276#2024411 <https://reviews.llvm.org/D79276#2024411>, @jhenderson wrote:

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

I'll probably wait until Monday to give others more time to participate.

Thanks for the review!

================
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'
----------------
jhenderson wrote:
> 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.
> I'm guessing the underscore is to circumvent lit trying to run things?

Yep.

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

I agree lit should change.  That might be the right condition.  People do things like the following, but maybe they shouldn't:

```
clang/test/Index/complete-access-checks-crash.cpp:    Derived(). // RUN: c-index-test -code-completion-at=%s:10:15 %s | FileCheck %s
```

At the very least, lit shouldn't recognize `RUN:` at the ends of other words (where `-` is considered a word character).  Lit even recognizes `XRUN:`.

================
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.
+
----------------
jhenderson wrote:
> 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.
I agree the use case is important, and I also agree there must be a better solution.

The underlying issue is that we want to reuse a pattern.  Perhaps there should be some way to define a FileCheck variable once and reuse it among multiple FileCheck commands. For example:

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

# ALL-DEF: [[VAR:some text that should be matched]]
# WITH: [[VAR]]
# WITHOUT-NOT: [[VAR]]
```

It should probably be possible to use regexes in such a variable.  I'm not sure if that's possible now.  It might require a special variable type.  We currently have `#` to indicate numeric variables.  Perhaps `~` would indicate pattern variables.

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

https://reviews.llvm.org/D79276