[PATCH] D79276: [FileCheck] Support comment directives

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

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


================
Comment at: llvm/lib/Support/FileCheck.cpp:1930
 
+bool FileCheck::ValidateCheckPrefixes() {
+  StringSet<> PrefixSet;
----------------
jhenderson wrote:
> It looks to me like the changes related to this function could be done separately (perhaps first). Is that the case, and if so, could you do so? It's a little hard to follow what's just refactoring of the existing stuff and what is new, with the extra comment stuff thrown in.
Good idea: D79375


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




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

https://reviews.llvm.org/D79276