[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

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

jhenderson wrote:
> 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.
Cool.  I think I could benefit from this feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9a9a5f9893c8: [FileCheck] Support comment directives 
(authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79276

Files:
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGenOpenCL/addr-space-struct-arg.cl
  clang/test/Driver/hip-device-libs.hip
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/test/Assembler/drop-debug-info-nonzero-alloca.ll
  llvm/test/FileCheck/comment/after-words.txt
  llvm/test/FileCheck/comment/bad-comment-prefix.txt
  llvm/test/FileCheck/comment/blank-comments.txt
  llvm/test/FileCheck/comment/suffixes.txt
  llvm/test/FileCheck/comment/suppresses-checks.txt
  llvm/test/FileCheck/comment/unused-check-prefixes.txt
  llvm/test/FileCheck/comment/unused-comment-prefixes.txt
  llvm/test/FileCheck/comment/within-checks.txt
  llvm/test/FileCheck/first-character-match.txt
  llvm/test/FileCheck/validate-check-prefix.txt
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -44,6 +44,14 @@
 cl::desc(
 "Alias for -check-prefix permitting multiple comma separated values"));
 
+static cl::list CommentPrefixes(
+"comment-prefixes", cl::CommaSeparated, cl::Hidden,
+cl::desc("Comma-separated list of comment prefixes to use from check file\n"
+ "(defaults to 'COM,RUN'). Please avoid using this feature in\n"
+ "LLVM's LIT-based test suites, which should be easier to\n"
+ "maintain if they all follow a consistent comment style. This\n"
+ "feature is meant for non-LIT test suites using FileCheck."));
+
 static cl::opt NoCanonicalizeWhiteSpace(
 "strict-whitespace",
 cl::desc("Do not treat all horizontal whitespace as equivalent"));
@@ -279,6 +287,8 @@
 return "label";
   case Check::CheckEmpty:
 return "empty";
+  case Check::CheckComment:
+return "com";
   case Check::CheckEOF:
 return "eof";
   case Check::CheckBadNot:
@@ -565,6 +575,9 @@
   for (StringRef Prefix : CheckPrefixes)
 Req.CheckPrefixes.push_back(Prefix);
 
+  for (StringRef Prefix : CommentPrefixes)
+Req.CommentPrefixes.push_back(Prefix);
+
   for (StringRef CheckNot : ImplicitCheckNot)
 Req.ImplicitCheckNot.push_back(CheckNot);
 
Index: llvm/test/FileCheck/validate-check-prefix.txt
===
--- llvm/test/FileCheck/validate-check-prefix.txt
+++ llvm/test/FileCheck/validate-check-prefix.txt
@@ -8,6 +8,6 @@
 
 ; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
 
-; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes: 'REPEAT'
+; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check and comment prefixes: 'REPEAT'
 
 ; EMPTY_PREFIX: error: supplied check prefix must not be the empty string
Index: llvm/test/FileCheck/first-character-match.txt
===
--- llvm/test/FileCheck/first-character-match.txt
+++ llvm/test/FileCheck/first-character-match.txt
@@ -1,2 +1,2 @@
-RUN: FileCheck -check-prefix=RUN -input-file %s %s
+RUN: FileCheck -check-prefix=RUN --comment-prefixes=COM -input-file %s %s
 // Prefix is at the first character in the file. The run line then matches itself.
Index: llvm/test/FileCheck/comment/within-checks.txt
===
--- /dev/null
+++ llvm/test/FileCheck/comment/within-checks.txt
@@ -0,0 +1,8 @@
+# A comment directive is not recognized within a check directive's pattern and
+# thus does not comment out the remainder of the pattern.
+
+RUN: echo 'foo' > %t.in
+RUN: echo 'CHECK: foo COM: bar' > %t.chk
+RUN: %ProtectFileCheckOutput not FileCheck %t.chk < %t.in 2>&1 | FileCheck %s
+
+CHECK: .chk:1:8: error: CHECK: expected string not found in input
Index: llvm/test/FileCheck/comment/unused-comment-prefixes.txt
===
--- /dev/null
+++ llvm/test/FileCheck/comment/unused-comment-prefixes.txt
@@ -0,0 +1,16 @@
+# Not using comment directives is always fine.
+
+RUN: echo 'foo'> %t.in
+RUN: echo 'CHECK: foo' > %t.chk
+
+# Check the case of default comment prefixes.
+RUN: %ProtectFileCheckOutput \
+RUN: FileCheck -vv %t.chk < %t.in 2>&1 | FileCheck %s
+
+# Specifying non-default comment prefixes doesn't mean you have to use them.
+# For example, they might be applied to an entire test suite via
+# FILECHECK_OPTS or via a wrapper command or substitution.
+RUN: %ProtectFileCheckOutput \
+RUN: FileCheck -vv 

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

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

Thanks!


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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Thomas Preud'homme via Phabricator via cfe-commits]

thopre accepted this revision.
thopre added a comment.

LGTM. Sorry for the late review.




Comment at: llvm/test/FileCheck/comment/after-words.txt:1
+# Comment prefixes are not recognized at ends of words.
+

jdenny wrote:
> jdenny wrote:
> > thopre wrote:
> > > How about characters that cannot be part of an identifier? E.g. would 
> > > "@COM:" be interpreted as a comment? Should we only accept comment prefix 
> > > if it's a space or beginning of line before?
> > Yes, this patch interprets `@COM:` as a comment directive.
> > 
> > My justification is that, to keep things simple for the FileCheck user and 
> > implementation, I tried to keep parsing of comment directives and check 
> > directives as similar as possible.  One intentional difference is that 
> > comment directives are ignored if they have check suffixes, such as `-NOT`.
> `git grep '@RUN'` shows that `@` without a following space might be a comment 
> style we should support. Here's an example match:
> 
> ```
> llvm/test/MC/ARM/ldr-pseudo-cond-darwin.s:@RUN: llvm-mc -triple 
> armv7-base-apple-darwin %s | FileCheck --check-prefix=CHECK-ARM 
> --check-prefix=CHECK %s
> ```
You're right, I forgot CHECK directives have the same problem. This should be 
fixed by the patch that was proposed to force a comment mark or beginning of 
line for a prefix to be recognized as a directive. Patch LGTM then.


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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added inline comments.



Comment at: llvm/test/FileCheck/comment/after-words.txt:1
+# Comment prefixes are not recognized at ends of words.
+

jdenny wrote:
> thopre wrote:
> > How about characters that cannot be part of an identifier? E.g. would 
> > "@COM:" be interpreted as a comment? Should we only accept comment prefix 
> > if it's a space or beginning of line before?
> Yes, this patch interprets `@COM:` as a comment directive.
> 
> My justification is that, to keep things simple for the FileCheck user and 
> implementation, I tried to keep parsing of comment directives and check 
> directives as similar as possible.  One intentional difference is that 
> comment directives are ignored if they have check suffixes, such as `-NOT`.
`git grep '@RUN'` shows that `@` without a following space might be a comment 
style we should support. Here's an example match:

```
llvm/test/MC/ARM/ldr-pseudo-cond-darwin.s:@RUN: llvm-mc -triple 
armv7-base-apple-darwin %s | FileCheck --check-prefix=CHECK-ARM 
--check-prefix=CHECK %s
```


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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added inline comments.



Comment at: llvm/test/FileCheck/comment/after-words.txt:1
+# Comment prefixes are not recognized at ends of words.
+

thopre wrote:
> How about characters that cannot be part of an identifier? E.g. would "@COM:" 
> be interpreted as a comment? Should we only accept comment prefix if it's a 
> space or beginning of line before?
Yes, this patch interprets `@COM:` as a comment directive.

My justification is that, to keep things simple for the FileCheck user and 
implementation, I tried to keep parsing of comment directives and check 
directives as similar as possible.  One intentional difference is that comment 
directives are ignored if they have check suffixes, such as `-NOT`.


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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Thomas Preud'homme via Phabricator via cfe-commits]

thopre added inline comments.



Comment at: llvm/test/FileCheck/comment/after-words.txt:1
+# Comment prefixes are not recognized at ends of words.
+

How about characters that cannot be part of an identifier? E.g. would "@COM:" 
be interpreted as a comment? Should we only accept comment prefix if it's a 
space or beginning of line before?


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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

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

In 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 

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: llvm/test/FileCheck/comment/blank-comments.txt:9
+
+CHECK: .chk:2:8: remark: CHECK: expected string found in input

jhenderson wrote:
> I'm assuming that FileCheck treats the entire line after the first `CHECK:` 
> here as part of the CHECK, and doesn't try to start a second CHECK directive?
That's right.



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:
> 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?



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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 262406.
jdenny marked 8 inline comments as done.
jdenny added a comment.

Addressed remaining reviewer comments except one, which is still under 
discussion.


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

https://reviews.llvm.org/D79276

Files:
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGenOpenCL/addr-space-struct-arg.cl
  clang/test/Driver/hip-device-libs.hip
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/test/Assembler/drop-debug-info-nonzero-alloca.ll
  llvm/test/FileCheck/comment/after-words.txt
  llvm/test/FileCheck/comment/bad-comment-prefix.txt
  llvm/test/FileCheck/comment/blank-comments.txt
  llvm/test/FileCheck/comment/suffixes.txt
  llvm/test/FileCheck/comment/suppresses-checks.txt
  llvm/test/FileCheck/comment/unused-check-prefixes.txt
  llvm/test/FileCheck/comment/unused-comment-prefixes.txt
  llvm/test/FileCheck/comment/within-checks.txt
  llvm/test/FileCheck/first-character-match.txt
  llvm/test/FileCheck/validate-check-prefix.txt
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -44,6 +44,14 @@
 cl::desc(
 "Alias for -check-prefix permitting multiple comma separated values"));
 
+static cl::list CommentPrefixes(
+"comment-prefixes", cl::CommaSeparated, cl::Hidden,
+cl::desc("Comma-separated list of comment prefixes to use from check file\n"
+ "(defaults to 'COM,RUN'). Please avoid using this feature in\n"
+ "LLVM's LIT-based test suites, which should be easier to\n"
+ "maintain if they all follow a consistent comment style. This\n"
+ "feature is meant for non-LIT test suites using FileCheck."));
+
 static cl::opt NoCanonicalizeWhiteSpace(
 "strict-whitespace",
 cl::desc("Do not treat all horizontal whitespace as equivalent"));
@@ -279,6 +287,8 @@
 return "label";
   case Check::CheckEmpty:
 return "empty";
+  case Check::CheckComment:
+return "com";
   case Check::CheckEOF:
 return "eof";
   case Check::CheckBadNot:
@@ -565,6 +575,9 @@
   for (StringRef Prefix : CheckPrefixes)
 Req.CheckPrefixes.push_back(Prefix);
 
+  for (StringRef Prefix : CommentPrefixes)
+Req.CommentPrefixes.push_back(Prefix);
+
   for (StringRef CheckNot : ImplicitCheckNot)
 Req.ImplicitCheckNot.push_back(CheckNot);
 
Index: llvm/test/FileCheck/validate-check-prefix.txt
===
--- llvm/test/FileCheck/validate-check-prefix.txt
+++ llvm/test/FileCheck/validate-check-prefix.txt
@@ -8,6 +8,6 @@
 
 ; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
 
-; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes: 'REPEAT'
+; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check and comment prefixes: 'REPEAT'
 
 ; EMPTY_PREFIX: error: supplied check prefix must not be the empty string
Index: llvm/test/FileCheck/first-character-match.txt
===
--- llvm/test/FileCheck/first-character-match.txt
+++ llvm/test/FileCheck/first-character-match.txt
@@ -1,2 +1,2 @@
-RUN: FileCheck -check-prefix=RUN -input-file %s %s
+RUN: FileCheck -check-prefix=RUN --comment-prefixes=COM -input-file %s %s
 // Prefix is at the first character in the file. The run line then matches itself.
Index: llvm/test/FileCheck/comment/within-checks.txt
===
--- /dev/null
+++ llvm/test/FileCheck/comment/within-checks.txt
@@ -0,0 +1,8 @@
+# A comment directive is not recognized within a check directive's pattern and
+# thus does not comment out the remainder of the pattern.
+
+RUN: echo 'foo' > %t.in
+RUN: echo 'CHECK: foo COM: bar' > %t.chk
+RUN: %ProtectFileCheckOutput not FileCheck %t.chk < %t.in 2>&1 | FileCheck %s
+
+CHECK: .chk:1:8: error: CHECK: expected string not found in input
Index: llvm/test/FileCheck/comment/unused-comment-prefixes.txt
===
--- /dev/null
+++ llvm/test/FileCheck/comment/unused-comment-prefixes.txt
@@ -0,0 +1,16 @@
+# Not using comment directives is always fine.
+
+RUN: echo 'foo'> %t.in
+RUN: echo 'CHECK: foo' > %t.chk
+
+# Check the case of default comment prefixes.
+RUN: %ProtectFileCheckOutput \
+RUN: FileCheck -vv %t.chk < %t.in 2>&1 | FileCheck %s
+
+# Specifying non-default comment prefixes doesn't mean you have to use them.
+# For example, they might be applied to an entire test suite via
+# FILECHECK_OPTS or via a wrapper command or substitution.
+RUN: %ProtectFileCheckOutput \
+RUN: FileCheck -vv 

[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/lib/Support/FileCheck.cpp:1928
 static const char *DefaultCheckPrefixes[] = {"CHECK"};
+static const char *DefaultCommentPrefixes[] = {"COM", "RUN"};
 

jdenny wrote:
> jhenderson wrote:
> > Similar to what I mentioned in D79375, perhaps it would make more sense for 
> > COM and RUN to be defined by the tool itself rather than the library to 
> > allow users to customise their respective front-ends as they wish.
> I suggested there that we handle the check prefix side of that change in a 
> separate patch.  Perhaps the same patch could handle the comment prefix side 
> of it.
> 
> Do you see a reason to handle this before committing the current patches?
I don't, as there's no current use-case for it. Let's leave it as-is for now.



Comment at: llvm/test/FileCheck/comment/bad-comment-prefix.txt:36
+RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
+RUN:  -check-prefixes=FOO,COM | \
+RUN:   FileCheck -check-prefix=CHECK-PREFIX-DUP-COMMENT %s

Perhaps worth checking against RUN as well as COM?



Comment at: llvm/test/FileCheck/comment/blank-comments.txt:9
+
+CHECK: .chk:2:8: remark: CHECK: expected string found in input

I'm assuming that FileCheck treats the entire line after the first `CHECK:` 
here as part of the CHECK, and doesn't try to start a second CHECK directive?



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

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)?



Comment at: llvm/test/FileCheck/comment/suppresses-checks.txt:1
+Comment directives successfully comment out check directives.
+

Missing '#'


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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 262185.
jdenny added a comment.

My last update accidentally included D79375 .  
This strips it back out.  Sorry about that.


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

https://reviews.llvm.org/D79276

Files:
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGenOpenCL/addr-space-struct-arg.cl
  clang/test/Driver/hip-device-libs.hip
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/test/Assembler/drop-debug-info-nonzero-alloca.ll
  llvm/test/FileCheck/comment/after-words.txt
  llvm/test/FileCheck/comment/bad-comment-prefix.txt
  llvm/test/FileCheck/comment/blank-comments.txt
  llvm/test/FileCheck/comment/suffixes.txt
  llvm/test/FileCheck/comment/suppresses-checks.txt
  llvm/test/FileCheck/comment/unused-check-prefixes.txt
  llvm/test/FileCheck/comment/unused-comment-prefixes.txt
  llvm/test/FileCheck/comment/within-checks.txt
  llvm/test/FileCheck/first-character-match.txt
  llvm/test/FileCheck/validate-check-prefix.txt
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -44,6 +44,14 @@
 cl::desc(
 "Alias for -check-prefix permitting multiple comma separated values"));
 
+static cl::list CommentPrefixes(
+"comment-prefixes", cl::CommaSeparated, cl::Hidden,
+cl::desc("Comma-separated list of comment prefixes to use from check file\n"
+ "(defaults to 'COM,RUN'). Please avoid using this feature in\n"
+ "LLVM's LIT-based test suites, which should be easier to\n"
+ "maintain if they all follow a consistent comment style. This\n"
+ "feature is meant for non-LIT test suites using FileCheck."));
+
 static cl::opt NoCanonicalizeWhiteSpace(
 "strict-whitespace",
 cl::desc("Do not treat all horizontal whitespace as equivalent"));
@@ -279,6 +287,8 @@
 return "label";
   case Check::CheckEmpty:
 return "empty";
+  case Check::CheckComment:
+return "com";
   case Check::CheckEOF:
 return "eof";
   case Check::CheckBadNot:
@@ -565,6 +575,9 @@
   for (StringRef Prefix : CheckPrefixes)
 Req.CheckPrefixes.push_back(Prefix);
 
+  for (StringRef Prefix : CommentPrefixes)
+Req.CommentPrefixes.push_back(Prefix);
+
   for (StringRef CheckNot : ImplicitCheckNot)
 Req.ImplicitCheckNot.push_back(CheckNot);
 
Index: llvm/test/FileCheck/validate-check-prefix.txt
===
--- llvm/test/FileCheck/validate-check-prefix.txt
+++ llvm/test/FileCheck/validate-check-prefix.txt
@@ -8,6 +8,6 @@
 
 ; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
 
-; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes: 'REPEAT'
+; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check and comment prefixes: 'REPEAT'
 
 ; EMPTY_PREFIX: error: supplied check prefix must not be the empty string
Index: llvm/test/FileCheck/first-character-match.txt
===
--- llvm/test/FileCheck/first-character-match.txt
+++ llvm/test/FileCheck/first-character-match.txt
@@ -1,2 +1,2 @@
-RUN: FileCheck -check-prefix=RUN -input-file %s %s
+RUN: FileCheck -check-prefix=RUN --comment-prefixes=COM -input-file %s %s
 // Prefix is at the first character in the file. The run line then matches itself.
Index: llvm/test/FileCheck/comment/within-checks.txt
===
--- /dev/null
+++ llvm/test/FileCheck/comment/within-checks.txt
@@ -0,0 +1,8 @@
+# A comment directive is not recognized within a check directive's pattern and
+# thus does not comment out the remainder of the pattern.
+
+RUN: echo 'foo' > %t.in
+RUN: echo 'CHECK: foo COM: bar' > %t.chk
+RUN: %ProtectFileCheckOutput not FileCheck %t.chk < %t.in 2>&1 | FileCheck %s
+
+CHECK: .chk:1:8: error: CHECK: expected string not found in input
Index: llvm/test/FileCheck/comment/unused-comment-prefixes.txt
===
--- /dev/null
+++ llvm/test/FileCheck/comment/unused-comment-prefixes.txt
@@ -0,0 +1,16 @@
+# Not using comment directives is always fine.
+
+RUN: echo 'foo'> %t.in
+RUN: echo 'CHECK: foo' > %t.chk
+
+# Check the case of default comment prefixes.
+RUN: %ProtectFileCheckOutput \
+RUN: FileCheck -vv %t.chk < %t.in 2>&1 | FileCheck %s
+
+# Specifying non-default comment prefixes doesn't mean you have to use them.
+# For example, they might be applied to an entire test suite via
+# FILECHECK_OPTS or via a wrapper command or substitution.
+RUN: %ProtectFileCheckOutput \
+RUN: FileCheck -vv 

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: llvm/lib/Support/FileCheck.cpp:1928
 static const char *DefaultCheckPrefixes[] = {"CHECK"};
+static const char *DefaultCommentPrefixes[] = {"COM", "RUN"};
 

jhenderson wrote:
> Similar to what I mentioned in D79375, perhaps it would make more sense for 
> COM and RUN to be defined by the tool itself rather than the library to allow 
> users to customise their respective front-ends as they wish.
I suggested there that we handle the check prefix side of that change in a 
separate patch.  Perhaps the same patch could handle the comment prefix side of 
it.

Do you see a reason to handle this before committing the current patches?



Comment at: llvm/lib/Support/FileCheck.cpp:1970
+  for (StringRef Prefix : Req.CommentPrefixes) {
+PrefixRegexStr.push_back('|');
+PrefixRegexStr.append(Prefix);

jhenderson wrote:
> This is incredibly nit-picky, but perhaps this should have a check for 
> `PrefixRegexStr.empty()`. It's such an edge case that it probably doesn't 
> matter, but assuming you adopt my comment about the default prefixes being 
> defined by the tool, you could imagine a situation where a different 
> front-end defined no check prefixes, and the user then didn't specify any 
> either.
Agreed.  As you say, it won't matter until tools can define default prefixes, 
so it can be addressed wherever that's addressed.



Comment at: llvm/test/FileCheck/comment.txt:1
+// Comment directives successfully comment out check directives.
+//

jhenderson wrote:
> jdenny wrote:
> > jhenderson wrote:
> > > You don't need to prefix the lines with '//' or anything else for that 
> > > matter, since this isn't being used as an assembly/yaml/C/etc input.
> > > 
> > > If you do want to have the characters (I don't care either way), I'd 
> > > prefer '#' for RUN/CHECK directive lines and '##' for comments (the 
> > > latter so they stand out better from directive lines). It's fewer 
> > > characters, whilst still as explicit ('#' is used widely in some parts of 
> > > the testsuite at least).
> > I don't much care either. The FileCheck test suite is not consistent in 
> > this regard.
> > 
> > What about the following style, making use of `COM:` (or `REM:` or whatever 
> > we choose) for comments as that's it's purpose:
> > 
> > ```
> > COM: 
> > -
> > COM: Comment directives successfully comment out check directives.
> > COM: 
> > -
> > 
> > COM: Check all default comment prefixes.
> > COM: Check that a comment directive at the begin/end of the file is handled.
> > COM: Check that the preceding/following line's check directive is not 
> > affected.
> > RUN: echo 'foo'   >  %t.in
> > RUN: echo 'COM: CHECK: bar'   >  %t.chk
> > RUN: echo 'CHECK: foo'>> %t.chk
> > RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
> > RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
> > RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
> > 
> > COM: Check the case of one user-specified comment prefix.
> > COM: Check that a comment directive not at the beginning of a line is 
> > handled.
> > RUN: echo 'foo'  >  %t.in
> > RUN: echo 'CHECK: foo'   >  %t.chk
> > RUN: echo 'letters then space MY-PREFIX: CHECK: bar' >> %t.chk
> > RUN: %ProtectFileCheckOutput \
> > RUN: FileCheck -vv %t.chk -comment-prefixes=MY-PREFIX < %t.in 2>&1 \
> > RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=1 %s
> > 
> > COM: Check the case of multiple user-specified comment prefixes.
> > RUN: echo 'foo'   >  %t.in
> > RUN: echo 'Bar_2: CHECK: Bar' >  %t.chk
> > RUN: echo 'CHECK: foo'>> %t.chk
> > RUN: echo 'Foo_1: CHECK: Foo' >> %t.chk
> > RUN: echo 'Baz_3: CHECK: Baz' >> %t.chk
> > RUN: %ProtectFileCheckOutput \
> > RUN: FileCheck -vv %t.chk -comment-prefixes=Foo_1,Bar_2 \
> > RUN:   -comment-prefixes=Baz_3 < %t.in 2>&1 \
> > RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
> > 
> > SUPPRESSES-CHECKS: .chk:[[CHECK_LINE]]:8: remark: CHECK: expected string 
> > found in input
> > ```
> Honestly, I don't find `COM:` stands out enough from other non-comment lines, 
> so I generally wouldn't use it unless I needed to workaround an issue, 
> although I'm not opposed to it in principle.
> 
> In this particular test, which is all about FileCheck's testing of the 
> comment directive, I'd suggest it might be best to avoid using it for 
> anything other than testing purposes to avoid any risk of confusion.
> In this particular test, which is all about FileCheck's testing of the 
> comment directive, I'd suggest it might be best to avoid using it for 
> anything other than testing purposes to avoid any risk of 

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 262141.
jdenny marked 14 inline comments as done.
jdenny added a comment.

Applied most reviewer comments.


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

https://reviews.llvm.org/D79276

Files:
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGenOpenCL/addr-space-struct-arg.cl
  clang/test/Driver/hip-device-libs.hip
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/test/Assembler/drop-debug-info-nonzero-alloca.ll
  llvm/test/FileCheck/comment/after-words.txt
  llvm/test/FileCheck/comment/bad-comment-prefix.txt
  llvm/test/FileCheck/comment/blank-comments.txt
  llvm/test/FileCheck/comment/suffixes.txt
  llvm/test/FileCheck/comment/suppresses-checks.txt
  llvm/test/FileCheck/comment/unused-check-prefixes.txt
  llvm/test/FileCheck/comment/unused-comment-prefixes.txt
  llvm/test/FileCheck/comment/within-checks.txt
  llvm/test/FileCheck/first-character-match.txt
  llvm/test/FileCheck/validate-check-prefix.txt
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -44,6 +44,14 @@
 cl::desc(
 "Alias for -check-prefix permitting multiple comma separated values"));
 
+static cl::list CommentPrefixes(
+"comment-prefixes", cl::CommaSeparated, cl::Hidden,
+cl::desc("Comma-separated list of comment prefixes to use from check file\n"
+ "(defaults to 'COM,RUN'). Please avoid using this feature in\n"
+ "LLVM's LIT-based test suites, which should be easier to\n"
+ "maintain if they all follow a consistent comment style. This\n"
+ "feature is meant for non-LIT test suites using FileCheck."));
+
 static cl::opt NoCanonicalizeWhiteSpace(
 "strict-whitespace",
 cl::desc("Do not treat all horizontal whitespace as equivalent"));
@@ -279,6 +287,8 @@
 return "label";
   case Check::CheckEmpty:
 return "empty";
+  case Check::CheckComment:
+return "com";
   case Check::CheckEOF:
 return "eof";
   case Check::CheckBadNot:
@@ -565,6 +575,9 @@
   for (StringRef Prefix : CheckPrefixes)
 Req.CheckPrefixes.push_back(Prefix);
 
+  for (StringRef Prefix : CommentPrefixes)
+Req.CommentPrefixes.push_back(Prefix);
+
   for (StringRef CheckNot : ImplicitCheckNot)
 Req.ImplicitCheckNot.push_back(CheckNot);
 
@@ -601,12 +614,8 @@
 Req.Verbose = true;
 
   FileCheck FC(Req);
-  if (!FC.ValidateCheckPrefixes()) {
-errs() << "Supplied check-prefix is invalid! Prefixes must be unique and "
-  "start with a letter and contain only alphanumeric characters, "
-  "hyphens and underscores\n";
+  if (!FC.ValidateCheckPrefixes())
 return 2;
-  }
 
   Regex PrefixRE = FC.buildCheckPrefixRegex();
   std::string REError;
Index: llvm/test/FileCheck/validate-check-prefix.txt
===
--- llvm/test/FileCheck/validate-check-prefix.txt
+++ llvm/test/FileCheck/validate-check-prefix.txt
@@ -1,10 +1,13 @@
 // RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=A! -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
 // RUN: FileCheck -check-prefix=A1a-B_c -input-file %s %s
-// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=REPEAT -check-prefix=REPEAT -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
+// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=REPEAT -check-prefix=REPEAT -input-file %s %s 2>&1 | FileCheck -check-prefix=DUPLICATE_PREFIX %s
 // RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=VALID -check-prefix=A! -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
-// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix= -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
+// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix= -input-file %s %s 2>&1 | FileCheck -check-prefix=EMPTY_PREFIX %s
 foobar
 ; A1a-B_c: foobar
 
-; BAD_PREFIX: Supplied check-prefix is invalid! Prefixes must be
-  unique and start with a letter and contain only alphanumeric characters, hyphens and underscores
+; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
+
+; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check and comment prefixes: 'REPEAT'
+
+; EMPTY_PREFIX: error: supplied check prefix must not be the empty string
Index: llvm/test/FileCheck/first-character-match.txt
===
--- llvm/test/FileCheck/first-character-match.txt
+++ llvm/test/FileCheck/first-character-match.txt
@@ -1,2 +1,2 @@
-RUN: FileCheck -check-prefix=RUN -input-file %s %s
+RUN: FileCheck -check-prefix=RUN --comment-prefixes=COM -input-file %s %s
 // Prefix is at 

[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/docs/CommandGuide/FileCheck.rst:57
+ By default, FileCheck ignores any occurrence in ``match-filename`` of any 
check
+ prefix if it is preceded on the same line by "``COM:``" or "``RUN:``".  See 
the
+ section `The "COM:" directive`_ for usage details.

jdenny wrote:
> jhenderson wrote:
> > Nit: the rest of this document uses single spaces after full stops. Please 
> > do that in the new text too.
> It doesn't matter to me which we use, especially given that it doesn't seem 
> to affect how the HTML renders.  However, the majority of periods (ignoring 
> `..` marking blocks) in this document that are followed by at least one space 
> are followed by two spaces. Does that change your suggestion?
Ah, it looks like it depends on who wrote the respective section. I know 
@probinson prefers double-spaces, and he wrote a lot of this stuff originally. 
I personally prefer single spaces (why waste extra characters etc etc). As the 
document is already inconsistent, I don't mind. When I made my original 
comment, I obviously didn't look at the right bits of the docs.



Comment at: llvm/lib/Support/FileCheck.cpp:1920
+  errs() << "error: supplied " << Kind << " prefix must be unique among "
+ << "check prefixes and comment prefixes: '" << Prefix << "'\n";
   return false;

This can probably be "check and comment prefixes"



Comment at: llvm/lib/Support/FileCheck.cpp:1928
 static const char *DefaultCheckPrefixes[] = {"CHECK"};
+static const char *DefaultCommentPrefixes[] = {"COM", "RUN"};
 

Similar to what I mentioned in D79375, perhaps it would make more sense for COM 
and RUN to be defined by the tool itself rather than the library to allow users 
to customise their respective front-ends as they wish.



Comment at: llvm/lib/Support/FileCheck.cpp:1970
+  for (StringRef Prefix : Req.CommentPrefixes) {
+PrefixRegexStr.push_back('|');
+PrefixRegexStr.append(Prefix);

This is incredibly nit-picky, but perhaps this should have a check for 
`PrefixRegexStr.empty()`. It's such an edge case that it probably doesn't 
matter, but assuming you adopt my comment about the default prefixes being 
defined by the tool, you could imagine a situation where a different front-end 
defined no check prefixes, and the user then didn't specify any either.



Comment at: llvm/test/FileCheck/comment.txt:1
+// Comment directives successfully comment out check directives.
+//

jdenny wrote:
> jhenderson wrote:
> > You don't need to prefix the lines with '//' or anything else for that 
> > matter, since this isn't being used as an assembly/yaml/C/etc input.
> > 
> > If you do want to have the characters (I don't care either way), I'd prefer 
> > '#' for RUN/CHECK directive lines and '##' for comments (the latter so they 
> > stand out better from directive lines). It's fewer characters, whilst still 
> > as explicit ('#' is used widely in some parts of the testsuite at least).
> I don't much care either. The FileCheck test suite is not consistent in this 
> regard.
> 
> What about the following style, making use of `COM:` (or `REM:` or whatever 
> we choose) for comments as that's it's purpose:
> 
> ```
> COM: -
> COM: Comment directives successfully comment out check directives.
> COM: -
> 
> COM: Check all default comment prefixes.
> COM: Check that a comment directive at the begin/end of the file is handled.
> COM: Check that the preceding/following line's check directive is not 
> affected.
> RUN: echo 'foo'   >  %t.in
> RUN: echo 'COM: CHECK: bar'   >  %t.chk
> RUN: echo 'CHECK: foo'>> %t.chk
> RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
> RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
> RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
> 
> COM: Check the case of one user-specified comment prefix.
> COM: Check that a comment directive not at the beginning of a line is handled.
> RUN: echo 'foo'  >  %t.in
> RUN: echo 'CHECK: foo'   >  %t.chk
> RUN: echo 'letters then space MY-PREFIX: CHECK: bar' >> %t.chk
> RUN: %ProtectFileCheckOutput \
> RUN: FileCheck -vv %t.chk -comment-prefixes=MY-PREFIX < %t.in 2>&1 \
> RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=1 %s
> 
> COM: Check the case of multiple user-specified comment prefixes.
> RUN: echo 'foo'   >  %t.in
> RUN: echo 'Bar_2: CHECK: Bar' >  %t.chk
> RUN: echo 'CHECK: foo'>> %t.chk
> RUN: echo 'Foo_1: CHECK: Foo' >> %t.chk
> RUN: echo 'Baz_3: CHECK: Baz' >> %t.chk
> RUN: %ProtectFileCheckOutput \
> RUN: FileCheck -vv %t.chk 

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 261998.
jdenny added a comment.

Fixed a missed rename in the test code in the last update.


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

https://reviews.llvm.org/D79276

Files:
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGenOpenCL/addr-space-struct-arg.cl
  clang/test/Driver/hip-device-libs.hip
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/test/Assembler/drop-debug-info-nonzero-alloca.ll
  llvm/test/FileCheck/comment.txt
  llvm/test/FileCheck/first-character-match.txt
  llvm/test/FileCheck/validate-check-prefix.txt
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -44,6 +44,14 @@
 cl::desc(
 "Alias for -check-prefix permitting multiple comma separated values"));
 
+static cl::list CommentPrefixes(
+"comment-prefixes", cl::CommaSeparated, cl::Hidden,
+cl::desc("Comma-separated list of comment prefixes to use from check file\n"
+ "(defaults to 'COM,RUN'). Please avoid using this feature in\n"
+ "LLVM's LIT-based test suites, which should be easier to\n"
+ "maintain if they all follow a consistent comment style. This\n"
+ "feature is meant for non-LIT test suites using FileCheck."));
+
 static cl::opt NoCanonicalizeWhiteSpace(
 "strict-whitespace",
 cl::desc("Do not treat all horizontal whitespace as equivalent"));
@@ -279,6 +287,8 @@
 return "label";
   case Check::CheckEmpty:
 return "empty";
+  case Check::CheckComment:
+return "com";
   case Check::CheckEOF:
 return "eof";
   case Check::CheckBadNot:
@@ -565,6 +575,9 @@
   for (StringRef Prefix : CheckPrefixes)
 Req.CheckPrefixes.push_back(Prefix);
 
+  for (StringRef Prefix : CommentPrefixes)
+Req.CommentPrefixes.push_back(Prefix);
+
   for (StringRef CheckNot : ImplicitCheckNot)
 Req.ImplicitCheckNot.push_back(CheckNot);
 
Index: llvm/test/FileCheck/validate-check-prefix.txt
===
--- llvm/test/FileCheck/validate-check-prefix.txt
+++ llvm/test/FileCheck/validate-check-prefix.txt
@@ -8,6 +8,6 @@
 
 ; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
 
-; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes: 'REPEAT'
+; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes and comment prefixes: 'REPEAT'
 
 ; EMPTY_PREFIX: error: supplied check prefix must not be the empty string
Index: llvm/test/FileCheck/first-character-match.txt
===
--- llvm/test/FileCheck/first-character-match.txt
+++ llvm/test/FileCheck/first-character-match.txt
@@ -1,2 +1,2 @@
-RUN: FileCheck -check-prefix=RUN -input-file %s %s
+RUN: FileCheck -check-prefix=RUN --comment-prefixes=COM -input-file %s %s
 // Prefix is at the first character in the file. The run line then matches itself.
Index: llvm/test/FileCheck/comment.txt
===
--- /dev/null
+++ llvm/test/FileCheck/comment.txt
@@ -0,0 +1,176 @@
+// Comment directives successfully comment out check directives.
+//
+// Check all default comment prefixes.
+// Check that a comment directive at the beginning/end of the file is handled.
+// Check that the preceding/following line's check directive is not affected.
+// RUN: echo 'foo'>  %t-supresses1.in
+// RUN: echo 'COM: CHECK: bar'>  %t-supresses1.chk
+// RUN: echo 'CHECK: foo' >> %t-supresses1.chk
+// RUN: echo 'RUN: echo "CHECK: baz"' >> %t-supresses1.chk
+// RUN: %ProtectFileCheckOutput \
+// RUN: FileCheck -vv %t-supresses1.chk < %t-supresses1.in 2>&1 \
+// RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
+//
+// Check the case of one user-specified comment prefix.
+// Check that a comment directive not at the beginning of a line is handled.
+// RUN: echo 'foo'  >  %t-suppresses2.in
+// RUN: echo 'CHECK: foo'   >  %t-suppresses2.chk
+// RUN: echo 'letters then space MY-PREFIX: CHECK: bar' >> %t-suppresses2.chk
+// RUN: %ProtectFileCheckOutput \
+// RUN: FileCheck -vv %t-suppresses2.chk -comment-prefixes=MY-PREFIX \
+// RUN:   < %t-suppresses2.in 2>&1 \
+// RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=1 %s
+//
+// Check the case of multiple user-specified comment prefixes.
+// RUN: echo 'foo'   >  %t-suppresses3.in
+// RUN: echo 'Bar_2: CHECK: Bar' >  %t-suppresses3.chk
+// RUN: echo 'CHECK: foo'>> %t-suppresses3.chk
+// RUN: echo 'Foo_1: CHECK: Foo' >> %t-suppresses3.chk

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: llvm/docs/CommandGuide/FileCheck.rst:57
+ By default, FileCheck ignores any occurrence in ``match-filename`` of any 
check
+ prefix if it is preceded on the same line by "``COM:``" or "``RUN:``".  See 
the
+ section `The "COM:" directive`_ for usage details.

jhenderson wrote:
> Nit: the rest of this document uses single spaces after full stops. Please do 
> that in the new text too.
It doesn't matter to me which we use, especially given that it doesn't seem to 
affect how the HTML renders.  However, the majority of periods (ignoring `..` 
marking blocks) in this document that are followed by at least one space are 
followed by two spaces. Does that change your suggestion?



Comment at: llvm/lib/Support/FileCheck.cpp:1930
 
+bool FileCheck::ValidateCheckPrefixes() {
+  StringSet<> PrefixSet;

jhenderson wrote:
> It looks to me like the changes related to this function could be done 
> separately (perhaps first). Is that the case, and if so, could you do so? 
> It's a little hard to follow what's just refactoring of the existing stuff 
> and what is new, with the extra comment stuff thrown in.
Good idea: D79375



Comment at: llvm/test/FileCheck/comment.txt:38-43
+// RUN: echo 'foo COM: bar'> %t.in
+// RUN: echo 'CHECK: foo COM: bar' > %t.chk
+// RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
+// RUN: | FileCheck -check-prefix=WITHIN-CHECKS %s
+//
+// WITHIN-CHECKS: .chk:1:8: remark: CHECK: expected string found in input

jhenderson wrote:
> I'm struggling a little with this case. Firstly, why the '8' in the column 
> count in the remark? Secondly, if COM: was being treated as a genuine comment 
> here, then the check directive would become `CHECK: foo` which would still be 
> a match of the input, if I'm not mistaken? I guess the two might somehow be 
> related, but I don't know how if so.
> Firstly, why the '8' in the column count in the remark?

Are you asking why it's not `1`?  Here, FileCheck gives the location of the 
first character of the pattern from the check file.

> Secondly, if COM: was being treated as a genuine comment here, then the check 
> directive would become CHECK: foo which would still be a match of the input, 
> if I'm not mistaken?

Ah, thanks.  Fixed.




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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 261962.
jdenny marked 11 inline comments as done.
jdenny added a comment.

Addressed most reviewer comments.


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

https://reviews.llvm.org/D79276

Files:
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGenOpenCL/addr-space-struct-arg.cl
  clang/test/Driver/hip-device-libs.hip
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/test/Assembler/drop-debug-info-nonzero-alloca.ll
  llvm/test/FileCheck/comment.txt
  llvm/test/FileCheck/first-character-match.txt
  llvm/test/FileCheck/validate-check-prefix.txt
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -44,6 +44,14 @@
 cl::desc(
 "Alias for -check-prefix permitting multiple comma separated values"));
 
+static cl::list CommentPrefixes(
+"comment-prefixes", cl::CommaSeparated, cl::Hidden,
+cl::desc("Comma-separated list of comment prefixes to use from check file\n"
+ "(defaults to 'COM,RUN'). Please avoid using this feature in\n"
+ "LLVM's LIT-based test suites, which should be easier to\n"
+ "maintain if they all follow a consistent comment style. This\n"
+ "feature is meant for non-LIT test suites using FileCheck."));
+
 static cl::opt NoCanonicalizeWhiteSpace(
 "strict-whitespace",
 cl::desc("Do not treat all horizontal whitespace as equivalent"));
@@ -279,6 +287,8 @@
 return "label";
   case Check::CheckEmpty:
 return "empty";
+  case Check::CheckComment:
+return "com";
   case Check::CheckEOF:
 return "eof";
   case Check::CheckBadNot:
@@ -565,6 +575,9 @@
   for (StringRef Prefix : CheckPrefixes)
 Req.CheckPrefixes.push_back(Prefix);
 
+  for (StringRef Prefix : CommentPrefixes)
+Req.CommentPrefixes.push_back(Prefix);
+
   for (StringRef CheckNot : ImplicitCheckNot)
 Req.ImplicitCheckNot.push_back(CheckNot);
 
Index: llvm/test/FileCheck/validate-check-prefix.txt
===
--- llvm/test/FileCheck/validate-check-prefix.txt
+++ llvm/test/FileCheck/validate-check-prefix.txt
@@ -8,6 +8,6 @@
 
 ; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
 
-; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes: 'REPEAT'
+; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes and comment prefixes: 'REPEAT'
 
 ; EMPTY_PREFIX: error: supplied check prefix must not be the empty string
Index: llvm/test/FileCheck/first-character-match.txt
===
--- llvm/test/FileCheck/first-character-match.txt
+++ llvm/test/FileCheck/first-character-match.txt
@@ -1,2 +1,2 @@
-RUN: FileCheck -check-prefix=RUN -input-file %s %s
+RUN: FileCheck -check-prefix=RUN --comment-prefixes=COM -input-file %s %s
 // Prefix is at the first character in the file. The run line then matches itself.
Index: llvm/test/FileCheck/comment.txt
===
--- /dev/null
+++ llvm/test/FileCheck/comment.txt
@@ -0,0 +1,175 @@
+// Comment directives successfully comment out check directives.
+//
+// Check all default comment prefixes.
+// Check that a comment directive at the beginning/end of the file is handled.
+// Check that the preceding/following line's check directive is not affected.
+// RUN: echo 'foo'>  %t-supresses1.in
+// RUN: echo 'COM: CHECK: bar'>  %t-supresses1.chk
+// RUN: echo 'CHECK: foo' >> %t-supresses1.chk
+// RUN: echo 'RUN: echo "CHECK: baz"' >> %t-supresses1.chk
+// RUN: %ProtectFileCheckOutput \
+// RUN: FileCheck -vv %t-supresses1.chk < %t-supresses1.in 2>&1 \
+// RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
+//
+// Check the case of one user-specified comment prefix.
+// Check that a comment directive not at the beginning of a line is handled.
+// RUN: echo 'foo'  >  %t-suppresses2.in
+// RUN: echo 'CHECK: foo'   >  %t-suppresses2.chk
+// RUN: echo 'letters then space MY-PREFIX: CHECK: bar' >> %t-suppresses2.chk
+// RUN: %ProtectFileCheckOutput \
+// RUN: FileCheck -vv %t-suppresses2.chk -comment-prefixes=MY-PREFIX \
+// RUN:   < %t-suppresses2.in 2>&1 \
+// RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=1 %s
+//
+// Check the case of multiple user-specified comment prefixes.
+// RUN: echo 'foo'   >  %t-suppresses3.in
+// RUN: echo 'Bar_2: CHECK: Bar' >  %t-suppresses3.chk
+// RUN: echo 'CHECK: foo'>> %t-suppresses3.chk
+// RUN: echo 'Foo_1: CHECK: Foo' >> 

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D79276#2017742 , @jdenny wrote:

> Thanks for the review.  I've replied to a few comments, and I'll address the 
> rest later.
>
> In D79276#2017101 , @jhenderson 
> wrote:
>
> > I hesitate to suggest this, but is it worth using `REM` as a comment 
> > prefix? It's the comment marker for Windows batch files, so has some 
> > precedence.
>
>
> I don't have a strong preference.  Does anyone else following this have a 
> preference?


One reason not to choose `REM:` is that it looks much more like `RUN:` than 
`COM:`.  For example, imagine the long code sample I wrote in my last comment 
, but with `COM:` replaced by 
`REM:`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79276



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

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

Thanks for the review.  I've replied to a few comments, and I'll address the 
rest later.

In D79276#2017101 , @jhenderson wrote:

> I hesitate to suggest this, but is it worth using `REM` as a comment prefix? 
> It's the comment marker for Windows batch files, so has some precedence.


I don't have a strong preference.  Does anyone else following this have a 
preference?

Some data about existing usage:

  $ cd llvm.git
  $ git grep '\ You don't need to prefix the lines with '//' or anything else for that 
> matter, since this isn't being used as an assembly/yaml/C/etc input.
> 
> If you do want to have the characters (I don't care either way), I'd prefer 
> '#' for RUN/CHECK directive lines and '##' for comments (the latter so they 
> stand out better from directive lines). It's fewer characters, whilst still 
> as explicit ('#' is used widely in some parts of the testsuite at least).
I don't much care either. The FileCheck test suite is not consistent in this 
regard.

What about the following style, making use of `COM:` (or `REM:` or whatever we 
choose) for comments as that's it's purpose:

```
COM: -
COM: Comment directives successfully comment out check directives.
COM: -

COM: Check all default comment prefixes.
COM: Check that a comment directive at the begin/end of the file is handled.
COM: Check that the preceding/following line's check directive is not affected.
RUN: echo 'foo'   >  %t.in
RUN: echo 'COM: CHECK: bar'   >  %t.chk
RUN: echo 'CHECK: foo'>> %t.chk
RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s

COM: Check the case of one user-specified comment prefix.
COM: Check that a comment directive not at the beginning of a line is handled.
RUN: echo 'foo'  >  %t.in
RUN: echo 'CHECK: foo'   >  %t.chk
RUN: echo 'letters then space MY-PREFIX: CHECK: bar' >> %t.chk
RUN: %ProtectFileCheckOutput \
RUN: FileCheck -vv %t.chk -comment-prefixes=MY-PREFIX < %t.in 2>&1 \
RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=1 %s

COM: Check the case of multiple user-specified comment prefixes.
RUN: echo 'foo'   >  %t.in
RUN: echo 'Bar_2: CHECK: Bar' >  %t.chk
RUN: echo 'CHECK: foo'>> %t.chk
RUN: echo 'Foo_1: CHECK: Foo' >> %t.chk
RUN: echo 'Baz_3: CHECK: Baz' >> %t.chk
RUN: %ProtectFileCheckOutput \
RUN: FileCheck -vv %t.chk -comment-prefixes=Foo_1,Bar_2 \
RUN:   -comment-prefixes=Baz_3 < %t.in 2>&1 \
RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s

SUPPRESSES-CHECKS: .chk:[[CHECK_LINE]]:8: remark: CHECK: expected string found 
in input
```



Comment at: llvm/test/FileCheck/comment.txt:2
+// Comment directives successfully comment out check directives.
+//
+// Check all default comment prefixes.

jhenderson wrote:
> Don't prefix empty lines.
I was doing that to keep related lines together in a block, but I can use 
"-" lines to separate blocks instead, as shown in my reply above.



Comment at: llvm/test/FileCheck/comment.txt:10-11
+// RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
+// RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
+// RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
+//

jhenderson wrote:
> I have a personal preference for multi-line commands like this to be 
> formatted like this:
> 
> ```
> // RUN:   ... | \
> // RUN: ...
> ```
> 
> with the `|` and `\` on the same line, to show that the next line is the 
> start of a new command (as opposed to just being more arguments for that 
> line), and the extra two space indentation showing that the line is a 
> continuation of the previous.
Has the community ever discussed a preferred style for the LLVM coding 
standards?  I don't have strong feelings about what we choose, but settling on 
one standard would be nice.



Comment at: llvm/test/FileCheck/comment.txt:143-147
+// Check user-supplied check prefix that duplicates a default comment prefix.
+// RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
+// RUN:  -check-prefixes=FOO,COM \
+// RUN: | FileCheck -check-prefix=CHECK-PREFIX-DUP-COMMENT %s
+// CHECK-PREFIX-DUP-COMMENT: error: supplied check prefix must be unique among 
check prefixes and comment prefixes: 'COM'

jhenderson wrote:
> I would naively expect --check-prefixes=FOO,COM to work, and disable the 
> comment prefix. Is that complicated to implement?
It's probably not complicated to implement.

However, there can 

[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

I hesitate to suggest this, but is it worth using `REM` as a comment prefix? 
It's the comment marker for Windows batch files, so has some precedence.




Comment at: llvm/docs/CommandGuide/FileCheck.rst:57
+ By default, FileCheck ignores any occurrence in ``match-filename`` of any 
check
+ prefix if it is preceded on the same line by "``COM:``" or "``RUN:``".  See 
the
+ section `The "COM:" directive`_ for usage details.

Nit: the rest of this document uses single spaces after full stops. Please do 
that in the new text too.



Comment at: llvm/lib/Support/FileCheck.cpp:1930
 
+bool FileCheck::ValidateCheckPrefixes() {
+  StringSet<> PrefixSet;

It looks to me like the changes related to this function could be done 
separately (perhaps first). Is that the case, and if so, could you do so? It's 
a little hard to follow what's just refactoring of the existing stuff and what 
is new, with the extra comment stuff thrown in.



Comment at: llvm/test/FileCheck/comment.txt:1
+// Comment directives successfully comment out check directives.
+//

You don't need to prefix the lines with '//' or anything else for that matter, 
since this isn't being used as an assembly/yaml/C/etc input.

If you do want to have the characters (I don't care either way), I'd prefer '#' 
for RUN/CHECK directive lines and '##' for comments (the latter so they stand 
out better from directive lines). It's fewer characters, whilst still as 
explicit ('#' is used widely in some parts of the testsuite at least).



Comment at: llvm/test/FileCheck/comment.txt:2
+// Comment directives successfully comment out check directives.
+//
+// Check all default comment prefixes.

Don't prefix empty lines.



Comment at: llvm/test/FileCheck/comment.txt:4
+// Check all default comment prefixes.
+// Check that a comment directive at the begin/end of the file is handled.
+// Check that the preceding/following line's check directive is not affected.

begin -> beginning



Comment at: llvm/test/FileCheck/comment.txt:9
+// RUN: echo 'CHECK: foo'>> %t.chk
+// RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
+// RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \

Is the lack of space after 'RUN:' deliberate?



Comment at: llvm/test/FileCheck/comment.txt:10-11
+// RUN: echo 'RUN:echo "CHECK: baz"' >> %t.chk
+// RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
+// RUN: | FileCheck -check-prefix=SUPPRESSES-CHECKS -DCHECK_LINE=2 %s
+//

I have a personal preference for multi-line commands like this to be formatted 
like this:

```
// RUN:   ... | \
// RUN: ...
```

with the `|` and `\` on the same line, to show that the next line is the start 
of a new command (as opposed to just being more arguments for that line), and 
the extra two space indentation showing that the line is a continuation of the 
previous.



Comment at: llvm/test/FileCheck/comment.txt:15
+// Check that a comment directive not at the beginning of a line is handled.
+// RUN: echo 'foo'  >  %t.in
+// RUN: echo 'CHECK: foo'   >  %t.chk

Not a big deal, but it might be easier for debugging the test in the future if 
the %t.in (and %t.chk) of each case is named differently, e.g. %t2.in, %t2.chk 
etc. That way, the later ones won't overwrite the earlier ones.



Comment at: llvm/test/FileCheck/comment.txt:38-43
+// RUN: echo 'foo COM: bar'> %t.in
+// RUN: echo 'CHECK: foo COM: bar' > %t.chk
+// RUN: %ProtectFileCheckOutput FileCheck -vv %t.chk < %t.in 2>&1 \
+// RUN: | FileCheck -check-prefix=WITHIN-CHECKS %s
+//
+// WITHIN-CHECKS: .chk:1:8: remark: CHECK: expected string found in input

I'm struggling a little with this case. Firstly, why the '8' in the column 
count in the remark? Secondly, if COM: was being treated as a genuine comment 
here, then the check directive would become `CHECK: foo` which would still be a 
match of the input, if I'm not mistaken? I guess the two might somehow be 
related, but I don't know how if so.



Comment at: llvm/test/FileCheck/comment.txt:127
+// RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
+// RUN:  -comment-prefixes= \
+// RUN: | FileCheck -check-prefix=PREFIX-EMPTY %s

Maybe worth cases like "-comment-prefixes=,FOO", "-comment-prefixes=FOO," and 
"-comment-prefixes=FOO,,BAR".



Comment at: llvm/test/FileCheck/comment.txt:133
+// RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
+// RUN:   -comment-prefixes=. \
+// RUN: | FileCheck -check-prefix=PREFIX-BAD-CHAR %s

[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: probinson, thopre, hfinkel, jhenderson, jroelofs, 
jdoerfert.
Herald added subscribers: cfe-commits, hiraditya, arichardson, Anastasia.
Herald added projects: clang, LLVM.

Sometimes you want to disable a FileCheck directive without removing
it entirely, or you want to write comments that mention a directive by
name.  The `COM:` directive makes it easy to do this.  For example,
you might have:

  ; X32: pinsrd_1:
  ; X32:pinsrd $1, 4(%esp), %xmm0
  
  ; COM: FIXME: X64 isn't working correctly yet for this part of codegen, but 
  ; COM: X64 will have something similar to X32:
  ; COM:
  ; COM:   X64: pinsrd_1:
  ; COM:   X64:pinsrd $1, %edi, %xmm0

Without this patch, you need to use some combination of rewording and 
directive syntax mangling to prevent FileCheck from recognizing the 
commented occurrences of `X32:` and `X64:` above as directives.
Moreover, FileCheck diagnostics have been proposed that might complain
about the occurrences of `X64` that don't have the trailing `:` 
because they look like directive typos:

  

I think dodging all these problems can prove tedious for test authors,
and directive syntax mangling already makes the purpose of existing
test code unclear.  `COM:` can avoid all these problems.

This patch also updates the small set of existing tests that define
`COM` as a check prefix:

- clang/test/CodeGen/default-address-space.c
- clang/test/CodeGenOpenCL/addr-space-struct-arg.cl
- clang/test/Driver/hip-device-libs.hip
- llvm/test/Assembler/drop-debug-info-nonzero-alloca.ll

I think lit should support `COM:` as well.  Perhaps `clang -verify`
should too.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79276

Files:
  clang/test/CodeGen/default-address-space.c
  clang/test/CodeGenOpenCL/addr-space-struct-arg.cl
  clang/test/Driver/hip-device-libs.hip
  llvm/docs/CommandGuide/FileCheck.rst
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/test/Assembler/drop-debug-info-nonzero-alloca.ll
  llvm/test/FileCheck/comment.txt
  llvm/test/FileCheck/first-character-match.txt
  llvm/test/FileCheck/validate-check-prefix.txt
  llvm/utils/FileCheck/FileCheck.cpp

Index: llvm/utils/FileCheck/FileCheck.cpp
===
--- llvm/utils/FileCheck/FileCheck.cpp
+++ llvm/utils/FileCheck/FileCheck.cpp
@@ -44,6 +44,14 @@
 cl::desc(
 "Alias for -check-prefix permitting multiple comma separated values"));
 
+static cl::list CommentPrefixes(
+"comment-prefixes", cl::CommaSeparated, cl::Hidden,
+cl::desc("Comma-separated list of comment prefixes to use from check file\n"
+ "(defaults to 'COM,RUN').  Please avoid using this feature in\n"
+ "LLVM's LIT-based test suites, which should be easier to\n"
+ "maintain if they all follow a consistent comment style.  This\n"
+ "feature is meant for non-LIT test suites using FileCheck."));
+
 static cl::opt NoCanonicalizeWhiteSpace(
 "strict-whitespace",
 cl::desc("Do not treat all horizontal whitespace as equivalent"));
@@ -279,6 +287,8 @@
 return "label";
   case Check::CheckEmpty:
 return "empty";
+  case Check::CheckComment:
+return "com";
   case Check::CheckEOF:
 return "eof";
   case Check::CheckBadNot:
@@ -565,6 +575,9 @@
   for (StringRef Prefix : CheckPrefixes)
 Req.CheckPrefixes.push_back(Prefix);
 
+  for (StringRef Prefix : CommentPrefixes)
+Req.CommentPrefixes.push_back(Prefix);
+
   for (StringRef CheckNot : ImplicitCheckNot)
 Req.ImplicitCheckNot.push_back(CheckNot);
 
@@ -601,12 +614,8 @@
 Req.Verbose = true;
 
   FileCheck FC(Req);
-  if (!FC.ValidateCheckPrefixes()) {
-errs() << "Supplied check-prefix is invalid! Prefixes must be unique and "
-  "start with a letter and contain only alphanumeric characters, "
-  "hyphens and underscores\n";
+  if (!FC.ValidateCheckPrefixes())
 return 2;
-  }
 
   Regex PrefixRE = FC.buildCheckPrefixRegex();
   std::string REError;
Index: llvm/test/FileCheck/validate-check-prefix.txt
===
--- llvm/test/FileCheck/validate-check-prefix.txt
+++ llvm/test/FileCheck/validate-check-prefix.txt
@@ -1,10 +1,13 @@
 // RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=A! -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
 // RUN: FileCheck -check-prefix=A1a-B_c -input-file %s %s
-// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=REPEAT -check-prefix=REPEAT -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
+// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=REPEAT -check-prefix=REPEAT -input-file %s %s 2>&1 | FileCheck -check-prefix=DUPLICATE_PREFIX %s
 // RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=VALID -check-prefix=A! -input-file %s %s