[PATCH] D79276: [FileCheck] Support comment directives

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added inline comments.


================
Comment at: llvm/test/FileCheck/comment/blank-comments.txt:9
+
+CHECK: .chk:2:8: remark: CHECK: expected string found in input
----------------
jhenderson wrote:
> 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?
That's right.


================
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.
+
----------------
jhenderson wrote:
> 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)?
> 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!

That's right, but check prefixes have this problem too.  That is, you can do things like `-check-prefixes=FOO,FOO-NOT` so that `FOO-NOT` is not negative.

`ValidatePrefixes` should be extended to catch such cases, I think.  But in a separate patch.

> 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)?

Hmm.  I think it's behavior we don't want to support.  Maybe the test case should be added when extending `ValidatePrefixes` as I described above?



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

https://reviews.llvm.org/D79276