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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D52999#1315065 <https://reviews.llvm.org/D52999#1315065>, @probinson wrote:

> 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.


No problem.  Thanks for the careful consideration.

>   But I've come down in favor of doing it.

Great!  I'll work on your suggestions soon.



================
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';
----------------
probinson wrote:
> 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.
> So, I can say -dump-input-on-failure -dump-input=fail and it will dump the input twice?

I was thinking -dump-input-on-failure would be removed eventually, so I decided to be lazy.  I'll change it.

> 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.

Do you want `-dump-input=never -dump-input-on-failure` to be the same as `-dump-input=never` or `-dump-input=fail`?





================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:486
+    errs() << '\n';
+    DumpInputAnnotationExplanation(errs(), Req);
+    std::vector<InputAnnotation> Annotations;
----------------
probinson wrote:
> 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.
> 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.


If the input dump is short, I usually don't need the dump.  I usually need the dump when it's long, and then the description is relatively tiny and doesn't feel like it's in the way.  Moreover, I don't want to force users to remember how to obtain that description (FileCheck isn't even normally in my PATH, so that's another barrier), so I think it's more convenient just to print it with the dump.

However, you're the second person with your opinion, so I'm outvoted, and I'm fine with that.  Any objection to always dumping a reminder that -dump-input=help exists?


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

https://reviews.llvm.org/D52999