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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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");
----------------
thopre wrote:
> 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?
No, as long as it is clear from the context, there's no need.


================
Comment at: llvm/lib/Support/FileCheck.cpp:190
       // supports @LINE, @LINE+number, @LINE-number expressions. The check here
       // is relaxed, more strict check is performed in \c EvaluateExpression.
+      if (IsPseudo) {
----------------
jhenderson wrote:
> Nit: is relaxed -> is a relaxed
Stricter -> A stricter


================
Comment at: llvm/lib/Support/FileCheck.cpp:1407
+  bool ErrorFound = false;
+  for (const auto &CmdlineDef : CmdlineDefines) {
+    std::string Prefix = "-D";
----------------
thopre wrote:
> 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?
Concatenation of StringRefs is supported via Twines. You can choose to make DiagCmdline a Twine itself, or do .str() on the result of the concatenation to get the final std::string a bit earlier, I don't really mind.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60382