[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.


================
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:
> > 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.
That's almost exactly what I've been prototyping on-and-off over the past few months, but I've been running into various ordering issues, which I haven't yet solved to my satisfaction. My original thread that inspired the idea is http://lists.llvm.org/pipermail/llvm-dev/2020-January/138822.html.


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

https://reviews.llvm.org/D79276