[PATCH] D79276: [FileCheck] Support comment directives

Mon May 4 08:32:43 PDT 2020

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 <https://reviews.llvm.org/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 '\<REM\(-[a-zA-Z0-9_-]*\)\?:' | wc -l
  115

In 21 matches (1 test file), `REM` is a FileCheck prefix.

In 4 matches (2 test files), `// REM:` is actually used to prefix a comment.  I'm not sure why.

The remaining 90 matches (29 test files) use `REM:` to define a FileCheck variable and shouldn't actually break behavior if `REM` is also a FileCheck comment prefix.  However, to make test code less confusing, people should probably avoid using such a FileCheck comment prefix for other purposes even if it works fine.

  $ git grep '\<COM\(-[a-zA-Z0-9_-]*\)\?:' | wc -l
  12

In 11 matches (4 test files), `COM` is a FileCheck prefix.  I fixed those as part of this patch.

In 1 match (1 test file), `CHECK-COM` is a FileCheck prefix.  No conflict there.

================
Comment at: llvm/test/FileCheck/comment.txt:1
+// Comment directives successfully comment out check directives.
+//
----------------
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
```

================
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: <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.
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 already be much confusion over check prefixes, and people keep proposing diagnostics to catch mistakes: which check prefixes are enabled, what text is intended to be directives vs. comments, undefined prefixes, unused prefixes, what prefixes are mistyped.  The motivation for a comment directive is to address part of this confusion.  But I don't want to extend the confusion along a new dimension by forcing people to have to also figure out what comment prefixes are in effect.  Instead, I think it would be good for LLVM test suites to settle on a single comment style and stick with it as much as possible.  The documentation I wrote specifically discourages using `-comment-prefixes` unless you're working in some test environment where the default comment prefixes just aren't appropriate.

Following this reasoning, I don't think it should be possible to quietly and perhaps accidentally override the default comment prefixes via `-check-prefixes`.  You should have to do so explicitly with `-comment-prefixes`.

What do you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79276