[PATCH] D79276: [FileCheck] Support comment directives

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1928
 static const char *DefaultCheckPrefixes[] = {"CHECK"};
+static const char *DefaultCommentPrefixes[] = {"COM", "RUN"};
 
----------------
jdenny wrote:
> 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?
I don't, as there's no current use-case for it. Let's leave it as-is for now.


================
Comment at: llvm/test/FileCheck/comment/bad-comment-prefix.txt:36
+RUN: %ProtectFileCheckOutput not FileCheck /dev/null < /dev/null 2>&1 \
+RUN:                                      -check-prefixes=FOO,COM | \
+RUN:   FileCheck -check-prefix=CHECK-PREFIX-DUP-COMMENT %s
----------------
Perhaps worth checking against RUN as well as COM?


================
Comment at: llvm/test/FileCheck/comment/blank-comments.txt:9
+
+CHECK: .chk:2:8: remark: CHECK: expected string found in input
----------------
I'm assuming that FileCheck treats the entire line after the first `CHECK:` here as part of the CHECK, and doesn't try to start a second CHECK directive?


================
Comment at: llvm/test/FileCheck/comment/suffixes.txt:1-2
+# Comment prefixes plus check directive suffixes are not comment directives
+# and are treated as plain text.
+
----------------
I don't think you should change anything here, but if I'm following this right, this leads to the amusing limitation that you can "comment out" (via --comment-prefixes) any CHECK lines that use a suffix (CHECK-NEXT, CHECK-NOT etc) but not those that don't without changing --check-prefixes value!

By the way, do we need to have a test-case for that? I.e. that --comment-prefixes=CHECK-NEXT disables the CHECK-NEXT lines (assuming it does of course)?


================
Comment at: llvm/test/FileCheck/comment/suppresses-checks.txt:1
+Comment directives successfully comment out check directives.
+
----------------
Missing '#'


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

https://reviews.llvm.org/D79276