[PATCH] D91761: [FileCheck] Add check modifier capability

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1683-1684
 
-  if (NextChar != '-')
+  // Verify that the : is present after the prefix or it is the start of
+  // directive modifiers.
+  if (Rest.front() == ':' || Rest.front() == '{')
----------------



================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1685-1686
+  // directive modifiers.
+  if (Rest.front() == ':' || Rest.front() == '{')
+    return ConsumeModifiers(Check::CheckPlain);
+
----------------
Looking at this again, I'm not sure these two should be folded together. I think it would make more sense to have the `:` check separate to the `{` check, and then not to call `ConsumeModifiers` in the event of the `:`, since there are none.


================
Comment at: llvm/test/FileCheck/check-literal.txt:20
+; A{LITERAL 5371, 5372,
+; A{LITERAL,} 5371, 5372,
+
----------------
Also `A{,LITERAL}`?


================
Comment at: llvm/test/FileCheck/check-literal.txt:30
+;; Modifiers are combined into a set and repetition is allowed.
+; A{LITERAL, LITERAL}: [[[5377]], [[5378]],
+
----------------
Also add a space before the comma, so that the before and after aspects of the whitespace trimming are tested.


================
Comment at: llvm/test/FileCheck/check-literal.txt:44
+
+; CHECK-ERRINV{LITTERAL}: 6371, 6372,
+; ERRINV: no check strings found with prefix 'CHECK-ERRINV
----------------
I'd make this more obvious, e.g.`INVALID`.


================
Comment at: llvm/test/FileCheck/check-literal.txt:31
+
+;; This ensures LITERAL applied to not in error cases reports an error.
+
----------------
thopre wrote:
> It took me a while to parse that sentence but I'm not sure how to improve it. Any suggestion @jhenderson ?
> 
> Perhaps "This ensures an error is correctly reported when a NOT directive with a LITERAL modifier matches"?
The latest version seems reasonable to me. I might suggest "an error" -> "a failure" since error could be implied to mean a problem (e.g. undefined variable) as opposed to a failed CHECK directive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91761