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

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

jdenny marked 21 inline comments as done.
jdenny added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:80-82
+.. option:: --dump-input <mode>
+
+  Dump annotated original input either 'always', on 'fail', or 'never'.
----------------
jdenny wrote:
> zturner wrote:
> > I haven't been following this too closely, but I'm wondering, are the 3 modes actually necessary?  It sounds like the main use case here is that the user has a failure and wants to get more info.  So they will set it to either `fail` or `always`.  But do they care which?  Basically, what I'm wondering is why not just make this be a binary on flag?
> > 
> > It just seems simpler to say that if you want to dump the input, pass `--dump-input`, and if you don't want to dump the input, pass nothing.  
> -dump-input=always is helpful when you're debugging individual tests and encounter FileCheck successes you don't understand.
> 
> I was thinking -dump-input=fail (via an env var) is better when running full test suites, perhaps from a bot or IDE.  Imagine a test with many successful FileCheck commands before the failed one.  -dump-input=always might produce massive output that will be logged and that must be scrolled/grepped through to find the failure, depending on how you like to interact with test suite logs.
> 
> In any case, -dump-input=fail was inspired by the existing -dump-input-on-failure, which I believe @george.karpenkov said is used in bots.  Is that right, George?
Seeing no followup for a while, I'm assuming my response here was satisfactory, and I'm marking this done.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1039
+        else
+          Diags->rbegin()->MatchTy = FileCheckDiag::MatchFinalButIllegal;
+      }
----------------
jdenny wrote:
> george.karpenkov wrote:
> > Five lines seem to be duplicated with the section above. Could that be extracted to a function? I also think that braces should not be avoided when "else" is present.
> > Five lines seem to be duplicated with the section above. Could that be extracted to a function?
> 
> Will do.
> 
> > I also think that braces should not be avoided when "else" is present.
> 
> That's a new rule to me.  It doesn't appear to be followed elsewhere in this file.  Is it followed elsewhere in LLVM?
Seeing no followup for a while, I'm assuming my response here was satisfactory, and I'm marking this done.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:232
+  for (auto DiagItr = DiagList.begin(), DiagEnd = DiagList.end();
+       DiagItr != DiagEnd; ++DiagItr) {
+    AnnotationList.emplace_back();
----------------
jdenny wrote:
> probinson wrote:
> > range-for here?
> D53893 (later patch in this series) makes use of the iterators.  Let me know if you think there's a better way. 
Seeing no followup for a while, I'm assuming my response here was satisfactory, and I'm marking this done.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52999