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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:664
+Directive modifiers
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
----------------
Fix this line to have the right number of `~` or the doc build will break. If you don't have it setup, I recommend you enable the docs build on your machine and build the html pages, to flush out any problems.

It would be nice to have an example of how to use the `LITERAL` modifier, both in conjunction with a normal check and a more complicated directive. I think this was there before, looking at the diff history, but it's disappeared again.


================
Comment at: llvm/include/llvm/FileCheck/FileCheck.h:96
+  }
+  FileCheckType &setLiteralMatch(bool literal = true) {
+    Modifiers.set(FileCheckKindModifier::ModifierLiteral, literal);
----------------
`literal` -> `Literal`, as per clang-tidy warning.

Same issue in various other cases in the new code (variables in the core LLVM project use `UpperCase`)


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1611
+  // Append directive modifiers.
+  auto withModifiers = [this, Prefix](const std::string &str) -> std::string {
+    return formatv("{0}{1}{2}", Prefix, str, getModifiersDescription());
----------------



================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:1612
+  auto withModifiers = [this, Prefix](const std::string &str) -> std::string {
+    return formatv("{0}{1}{2}", Prefix, str, getModifiersDescription());
+  };
----------------
Would it be simpler to just do `Prefix + str + getModifiersDescription()`? Not sure `formatv` really gets us anything here.


================
Comment at: llvm/test/FileCheck/check-literal.txt:1
+; RUN: FileCheck -check-prefix=A -input-file %s %s
+
----------------
It would be nice to have a brief comment at the start of the test explaining what this test is actually testing (specifically, it's testing the literal modifier, and not something else that might be considered "literal").

You should probably have a case with a suffix following `{LITERAL}` to show the behaviour when the `:` doesn't immediately follow it. Also, where there's no closing `}` after `LITERAL`.

Is it worth checking the use with `-NOT` too? Not sure on that one, but maybe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91761