[PATCH] D79276: [FileCheck] Support comment directives
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 5 01:34:34 PDT 2020
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 -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.
================
Comment at: llvm/test/FileCheck/comment.txt:2
+// Comment directives successfully comment out check directives.
+//
+// Check all default comment prefixes.
----------------
jdenny wrote:
> 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.
Ah, I see what you are doing. Maybe it would just be better to split the related groups into separate test files. That'll make them run faster when run in isolation, and allow for easier debugging too, whilst also keeping relateed stuff clearly together.
================
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
+//
----------------
jdenny wrote:
> 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.
I don't think there's ever been a discussion. Feel free to start one, and I'll chime in.
================
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
----------------
jdenny wrote:
> 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.
>
>
> Are you asking why it's not `1`? Here, FileCheck gives the location of the first character of the pattern from the check file.
Apparently I can't count. Thanks.
================
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'
----------------
jdenny wrote:
> 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?
Okay, that sounds reasonable to me. I guess it should be fairly obvious what is going on.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79276/new/
https://reviews.llvm.org/D79276
More information about the llvm-commits
mailing list