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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

Could you add some more tests:

- negative tests for the modifier syntax (e.g. "{LITERAL,}", "{}", "{LITTERAL}", "{LITERAL"
- positive tests with several modifiers (i.e. "{LITERAL,LITERAL}")

It's looking good otherwise. Thanks for making the code already prepared for multiple modifiers. I think we might want to have a shorter version of the modifier but better keep it for later when the need is felt.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:667
+A directive modifier can be append to a directive by following the directive
+with ``{modifier}``. Where modifier can be one of the following:
+
----------------
I suggest to match the formalism used to introduced numeric expression, as per edit


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:669
+
+LITERAL
+  The ``LITERAL`` directive modifier can be used to perform a literal match. The
----------------
When there'll be more than one I'll suggest using a list but since there's only one supported value I propose folding it inside the text above (as per edit)


================
Comment at: llvm/include/llvm/FileCheck/FileCheck.h:73
+  // The number of modifier.
+  Size = 1
+};
----------------
I'd remove the value, it'll automatically take the last enumerator + 1 and won't require changing for every new modifier.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1668-1673
+    do {
+      if (Rest.consume_front("LITERAL"))
+        Ret.setLiteralMatch();
+      else
+        return {Check::CheckNone, Rest};
+    } while (Rest.consume_front(","));
----------------
Perhaps allow whitespaces?


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1679
+
   // Verify that the : is present after the prefix.
+  if (Rest.front() == ':' || Rest.front() == '{')
----------------
This comment needs updating


================
Comment at: llvm/test/FileCheck/check-literal.txt:31
+
+;; This ensures LITERAL applied to not in error cases reports an error.
+
----------------
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"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91761