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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
