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

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
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';
----------------
jdenny wrote:
> 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`?
> 
> 
> 
cl::opt doesn't support command-line-order checks, so what I'd do is have the -dump-input enum have a Default. Then if -dump-input is Default, you set it to Never or Fail depending on -dump-input-on-failure.  If/when we get rid of -dump-input-on-failure, we can set the -dump-input default to Never and get rid of the Default enum too.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:486
+    errs() << '\n';
+    DumpInputAnnotationExplanation(errs(), Req);
+    std::vector<InputAnnotation> Annotations;
----------------
jdenny wrote:
> 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?
A reminder that -dump-input=help exists would be totally appropriate.


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

https://reviews.llvm.org/D52999