[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via llvm-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: <command> <args> ... | \
// RUN:   <next command> <args> ...
```

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
----------------
Maybe worth an additional case where the invalid character is not the first character in the string.


================
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'
----------------
I would naively expect --check-prefixes=FOO,COM to work, and disable the comment prefix. Is that complicated to implement?


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:50
+    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"
----------------
Similar comment to earlier. Every multi-sentence diagnostic I've seen in LLVM uses single spaces, so this probably should too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79276