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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with a couple of nits.



================
Comment at: llvm/lib/Support/FileCheck.cpp:1409
+  // definition is on its own line and prefixed with a definition number to
+  // clarify which definition does a given diagnostic correspond to.
+  unsigned I = 0;
----------------
"which definition a given diagnostic corresponds to."


================
Comment at: llvm/lib/Support/FileCheck.cpp:1412
+  bool ErrorFound = false;
+  std::string CmdlineDefsDiag = std::string();
+  StringRef Prefix1 = "Global define #";
----------------
You don't need `= std::string()` here.


================
Comment at: llvm/test/FileCheck/defines.txt:38
+
+; ERRCLIFMT: Global defines:1:1: error: Invalid name for variable definition '10VALUE'
+; ERRCLIFMT-NEXT: 10VALUE=10
----------------
thopre wrote:
> jhenderson wrote:
> > The line and column numbers like a bit weird to me, but isn't a big deal. If there's a way to not have them though, it would be better. Alternatively, if they have some actual meaning, please have a test case that produces different values for them.
> Not have them in the output or in this expected text?
> 
> If the former yes currently it's not meaningful because the current code create a separate buffer with same name (Global defines) for each definition, so all definition would be a line 1, and the name would be a column 1 (I think it's good to test that bit since it shows we point to the variable name rather than the value).
> I could change the code to have one line per definition which could be useful if a variable is defined twice from the command-line (i.e. -DFOO=10 -DFOO=12). Alternatively I could make the definition one after the other on the same line, perhaps separated by space (i.e. output would be FOO=10 FOO=12 with the caret pointing to the right FOO). Both of these approaches will require to loop over definition twice (once to construct the buffer, the second to parse it).
> 
> If the latter, I cannot introduce variations because the name will always be first but I think it's worth testing that the location is accurate.
I was referring to the output, but I think this looks somewhat better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60382