[PATCH] D52999: [FileCheck] Annotate input dump

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

jdenny added a comment.

In https://reviews.llvm.org/D52999#1258255, @george.karpenkov wrote:

> I've added some nits inline.


Thanks for reviewing.

> Overall, this is a huge patch, and I still have trouble understanding all of what it is doing, so I think it should be broken up.

I'm happy to do so, but let's agree on the right way to break it up before I put in the effort.  Here's my proposed patch sequence:

1. Make changes to include/llvm/Support/FileCheck.h and lib/Support/FileCheck.cpp, which gather the list of diagnostics.
2. Make changes to utils/FileCheck/FileCheck.cpp, which converts the list of diagnostics to annotations on the input.
3. Adjust command-line option and env var.
4. Expose colorsEnabled in WithColor.h.

On second thought, these are not logically distinct changes as none of them really has much purpose on its own.  You might just consider reviewing files in the above order.  Your call.  Let me know.

> As to testing colors, I think that if the platform is fixed, just checking the color codes should be fine?

You mean ANSI escape sequences?  That would make for some rather unreadable expected output.  There must be a better way.  If only we had a tool that converts those sequences into something human-readable, like XML tags.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:112
+                       const StringMap<StringRef> &VariableTable,
+                       std::list<FileCheckDiag> *Diags) const;
 
----------------
george.karpenkov wrote:
> IMO to be consistent, this parameter should also be a reference
It might be a nullptr.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1039
+        else
+          Diags->rbegin()->MatchTy = FileCheckDiag::MatchFinalButIllegal;
+      }
----------------
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?


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:116
+  raw_ostream::Colors Color;
+  unsigned RequiredVerbosity; // 0 = none, 1 = -v, 2 = -vv
+  const char *What;
----------------
george.karpenkov wrote:
> Should this be an enum then?
Will do.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:163
+  if (WithColor::colorsEnabled(errs()))
+    errs().changeColor(raw_ostream::BLACK, true);
+  errs() << "L";
----------------
george.karpenkov wrote:
> `with / changeColor`  block is duplicated many times, should be extracted into a function.
Will do.


Repository:
  rL LLVM

https://reviews.llvm.org/D52999