[PATCH] D52999: [FileCheck] Annotate input dump

[George Karpenkov via Phabricator via llvm-commits]

george.karpenkov added a comment.

I've added some nits inline.
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.

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



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


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


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


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


Repository:
  rL LLVM

https://reviews.llvm.org/D52999