[PATCH] D114684: Add ScopedPrinter unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Nearly there, just one actual comment.



================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:86
+TEST_F(ScopedPrinterTest, PrintIndent) {
+  auto ResetIndentFunc = [](raw_string_ostream &OS, ScopedPrinter &W) {
+    W.printIndent();
----------------
Variable names in this test seem wrong.

I wonder if you should change all the expected strings to be called `ExpectedOut`? And similarly perhaps the functions could have a common name (I don't have any concrete suggestions yet)?


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:392
+      const std::vector<std::string> StringList = {"foo", "bar", "baz"};
+      const bool BoolList[] = {true, false};
+      const std::vector<uint64_t> Unsigned64List = {
----------------
//Mutters about silly `std::vector<bool>` standard specialization...//


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:613
+  auto StartLineFunc = [](raw_string_ostream &OS, ScopedPrinter &W) {
+    W.startLine() << "|";
+    W.indent(2);
----------------
`startLine` not adding a new line implicitly is not what I expected!


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:124-125
+      DictScope D(W);
+      W.printEnum("Exists", EnumList[1].Value, makeArrayRef(EnumList));
+      W.printEnum("DoesNotExist", OtherEnum.Value, makeArrayRef(EnumList));
+    }
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > More unnecessary(?) `makeArrayRef` calls.
> After trying for a bit, I don't think I can get rid of these `makeArrayRef` calls. I think it can't implicitly cast to an `ArrayRef` because the `printEnum` method takes in a template list rather than a list of concrete type. 
Okay, thanks for the explanation.


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

https://reviews.llvm.org/D114684