[PATCH] D60382: FileCheck [2/12]: Stricter parsing of -D option

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

thopre added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:171
+      if (parseVariable(MatchStr, IsPseudo, TrailIdx)) {
+        SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data()),
+                        SourceMgr::DK_Error, "Invalid name in named regex");
----------------
jhenderson wrote:
> Does this print the actual specified string in the error message?
It'll print the line, followed by a caret pointing to the variable followed by the error message. I thought mentioning the variable name in the error message was superfluous given the caret. Do you want me to include it?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1407
+  bool ErrorFound = false;
+  for (const auto &CmdlineDef : CmdlineDefines) {
+    std::string Prefix = "-D";
----------------
jhenderson wrote:
> Keep the `StringRef` here.
Should I rather use std::string instead? I need to prefix each string with "-D" (see DiagCmdline = Prefix + CmdlineDef) which cannot be done with a reference. Is there anything wrong with using std::string?


================
Comment at: llvm/lib/Support/FileCheck.cpp:1408
+  for (const auto &CmdlineDef : CmdlineDefines) {
+    std::string Prefix = "-D";
+    std::string DiagCmdline = Prefix + CmdlineDef;
----------------
jhenderson wrote:
> Use a StringRef here.
Likewise, I need this to be std::string to allow for the concatenation on next line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60382