[PATCH] D79276: [FileCheck] Support comment directives

Tue May 5 09:41:47 PDT 2020

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

Fair enough. We can do something different for this test. But I am interested in pursuing more discussion on the general appeal of `COM:`....

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

By that logic, the example from this patch's summary might end up like this:

```
; X32: pinsrd_1:
; X32:    pinsrd $1, 4(%esp), %xmm0

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

After more FileCheck diagnostics land, the `FIXME` line might break, and a new `COM:` would have to be added.

Instead, my hope was that people would start to see `COM:` (usually with a leading `;`, `//`, etc.) as a general way to style entire comment blocks in test files.  Once that habit is in place and eyes are trained, true comments would become very obvious, and test authors would never have to think about whether FileCheck would trip on a comment that looks like a directive.

Of course, I'm not suggesting an overhaul of all test suites once this lands.  I'm just suggesting people would use it more and more to improve readability of tests, both for humans and for tools like FileCheck.

Is there anything we can do to improve the appeal of `COM:`? Or is it just a matter of getting used to it, as with any comment style?

================
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:
> 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.
For this patch, I've switched to the style you suggested.

I'm afraid that leading a general discussion on that issue is low on my list of priorities.  Maybe some day.

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

https://reviews.llvm.org/D79276