[PATCH] D114684: Add ScopedPrinter unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Looks like you're still missing tests for `startLine` and `getOStream`, which should be simple, but for completeness should be added too.



================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:29
+
+TEST_F(ScopedPrinterTest, PrintIndent) {
+  auto IndentFunc = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
Now that you've added separate indent/unindent tests, I'd move `PrintIndent` to be later in this file, to match the function declaration order.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:46
+
+  auto UnindentFunc = [](raw_string_ostream &OS, ScopedPrinter &W) {
+    W.indent();
----------------
I'd recommend one function under test per TEST, i.e. split this and ResetIndent into their own TESTs.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:89-90
+  W.unindent();
+  EXPECT_EQ(W.getIndentLevel(), 1);
+  W.resetIndent();
+  EXPECT_EQ(W.getIndentLevel(), 0);
----------------
Perhaps add one more `indent` call between these two lines, so that a) you show that you can indent after an unindent, and b) show that reset is not just an unindent alias, but rather a complete reset.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:93
+  W.unindent();
+  EXPECT_EQ(W.getIndentLevel(), 0);
+}
----------------
I'd be tempted to put one more `indent` at the end here, to show that the internal representation is really capped at 0, rather than just `getIndentLevel`'s external result.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:103-104
+    W.resetIndent();
+    W.printString("|");
+    W.flush();
+    return OS.str();
----------------
Perhaps add another `setPrefix` call followed by a print here, to show that the existing prefix can be changed.

I'd also consider calling `printIndent` in addition to, or instead of `printString`, since `printIndent` explicitly adds the prefix itself.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:120
+                                       {"Name3", "AltName3", 3},
+                                       {"Name4", "AltName4", 1}};
+    EnumEntry<int> OtherEnum{"Name5", "AltName5", 5};
----------------
As the value you look up is actually "2", you need this duplicate case to match it, so that you still show the "first one wins" behaviour.


================
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));
+    }
----------------
More unnecessary(?) `makeArrayRef` calls.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:153
+      DictScope D(W);
+      W.printFlags("ZeroFlag", 0, makeArrayRef(SingleBitFlags));
+      W.printFlags("NoFlag", 1 << 3, makeArrayRef(SingleBitFlags));
----------------
Lots of `makeArrayRef` calls in this test, which could be simplified by using `std::vector` or other containers.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:171-178
+      W.printFlags("FirstByteMask", 0x01u, makeArrayRef(EnumFlags),
+                   FirstByteMask);
+      W.printFlags("SecondByteMask", 0x10u, makeArrayRef(EnumFlags),
+                   SecondByteMask);
+      W.printFlags("ValueOutsideMask", 0x02u, makeArrayRef(EnumFlags),
+                   SecondByteMask);
+      W.printFlags("FirstSecondByteMask", 0xFFu, makeArrayRef(EnumFlags),
----------------
Not wanting to be too picky, but this still doesn't exercise all arguments (there are three `EnumMask` arguments, and you only test with two).


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:51
+    {
+      DictScope D(W);
+      uint64_t Unsigned64Max = std::numeric_limits<uint64_t>::max();
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > What's the reasoning for the `DictScope`? I don't see anything in the code that requires it, and it just confuses the test a bit.
> It's not required for the `ScopedPrinter` but it is for `JSONScopedPrinter`. It's possible to remove but it would just mean that we wouldn't be able to reuse this lambda for the `JSONScopedPrinter` equivalent test.
Okay, leave as-is.


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

https://reviews.llvm.org/D114684