[PATCH] D52999: [FileCheck] Annotate input dump (1/7)

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Apologies for the delay.  I haven't been ignoring this series; I was having internal qualms about the amount of effort to produce extensive annotations, and the value they might provide.  But I've come down in favor of doing it.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:90
+  When the check fails, dump all of the original input.  This option is
+  deprecated in favor of `--dump-input`.
 
----------------
in favor of `--dump-input=fail`.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:102
+             "or 'never'.\n"),
+    cl::value_desc("mode"));
 
----------------
There's a way to make the argument be an enum, which has a variety of advantages.  Please do it that way.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:146
+  WithColor(OS, raw_ostream::SAVEDCOLOR, true) << "L:";
+  OS << "S    labels line number L of the input file, where S is a "
+     << "single space\n";
----------------
I'd omit the `S` part.  `6: ` is clearly a line number, you don't need to document that the colon has a space after it.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:479
+  std::vector<FileCheckDiag> Diags;
+  int ExitCode = FC.CheckInput(SM, InputFileText, CheckStrings, &Diags)
+                     ? EXIT_SUCCESS
----------------
`DumpInput == "never" ? nullptr : &Diags`
so we don't bother collecting diags that we will never print. Saves a small bit of time and memory, but this tool is used a *lot* in the default "never" mode and it's worth doing that small optimization.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:484
     errs() << "Full input was:\n<<<<<<\n" << InputFileText << "\n>>>>>>\n";
+  if (DumpInput == "always" || (ExitCode == 1 && DumpInput == "fail")) {
+    errs() << '\n';
----------------
So, I can say `-dump-input-on-failure -dump-input=fail` and it will dump the input twice?  I think `-dump-input-on-failure` should just set `-dump-input=fail` (if `-dump-input` didn't appear separately, i.e. the new option takes precedence) and you only get one dump.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:486
+    errs() << '\n';
+    DumpInputAnnotationExplanation(errs(), Req);
+    std::vector<InputAnnotation> Annotations;
----------------
The detailed description of the annotations becomes long enough that I think including it with the dumped input starts to get in the way.  Maybe have a `-dump-input=help` that will print the description and quit, or something along those lines.


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

https://reviews.llvm.org/D52999