[PATCH] D114684: Add ScopedPrinter unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:21
+
+  void verifyScopedPrinter(StringRef Expected, PrintFcn Fcn) {
+    std::string StreamBuffer;
----------------
I don't know what "Fcn" stands for. At a guess, it's either "function" or "functor" or similar. I'd suggest "Func" maybe? Similar throughout.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:31
+  auto IndentFcn = [](raw_string_ostream &OS, ScopedPrinter &W) {
+    W.indent();
+    W.printString("|");
----------------
Perhaps avoid the first `indent`, so that we can see what the initial state is like (or print a string before it).

You also need a test case for unindenting when nothing is left to unindent.

Also missing tests for `resetIndent`, `getIndent` and other similar functions that are in the public interface.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:48
+
+TEST_F(ScopedPrinterTest, PrintNumber) {
+  auto NumberFcn = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
I'd suggest ordering tests in the same order as the functions in the class.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:51
+    {
+      DictScope D(W);
+      uint64_t Unsigned64Max = std::numeric_limits<uint64_t>::max();
----------------
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.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:128
+                                       {"Name2", "AltName2", 2},
+                                       {"Name3", "AltName3", 3}};
+    EnumEntry<int> OtherEnum{"Name4", "AltName4", 4};
----------------
It may be useful to have an entry in this list with a duplicate value as an earlier one, to show that "first one wins" is the approach taken.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:132
+      DictScope D(W);
+      W.printEnum("Exists", EnumList[0].Value, makeArrayRef(EnumList));
+      W.printEnum("DoesNotExist", OtherEnum.Value, makeArrayRef(EnumList));
----------------
I'd suggest using index 1, to show you're not just giving up if the first element doesn't match.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:148
+  auto FlagFcn = [](raw_string_ostream &OS, ScopedPrinter &W) {
+    const EnumEntry<int> FlagList[] = {{"Name1", "AltName1", 1},
+                                       {"Name2", "AltName2", 1 << 1},
----------------
I think you need a flag with value 0 in this list, to exercies the `if ... continue` part.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:153-155
+      W.printFlags("NoFlag", 1 << 3, makeArrayRef(FlagList));
+      W.printFlags("Flag1", FlagList[0].Value, makeArrayRef(FlagList));
+      W.printFlags("Flag1&3", (1 << 2) + 1, makeArrayRef(FlagList));
----------------
You need test cases for when the other input parameters of this function are not the default values.

I believe you may also need soemthing that shows the `sort` call exists.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:221
+
+TEST_F(ScopedPrinterTest, PrintList) {
+  auto ListFcn = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
`printList` has an overload which takes an additional `Printer` parameter which doesn't seem to be tested here.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:244
+
+TEST_F(ScopedPrinterTest, PrintHex) {
+  auto HexFcn = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
I'd split this into three different tests for the three different function names.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:252
+      W.printHex("HexLabel", "Name", 0x10);
+      W.printSymbolOffset("SymbolOffset", "SymbolName", 0x10);
+    }
----------------
Might be interesting to show that a 0 offset is still explicitly printed.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:278-279
+      ListScope L(W, "StringList");
+      W.printString(StringRefValue);
+      W.printString(StringRefValue);
+    }
----------------
The fact that these two cases are inside the ListScope makes the ListScope look like it's important to the behaviour here, but I don't think it is?

Also, why print twice?


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:303
+      const char CharArray[] = {'F', 'o', 'o', 'B', 'a', 'r'};
+      W.printBinary("Binary1", "FooBar", makeArrayRef(IntArray));
+      W.printBinary("Binary2", "FooBar", makeArrayRef(CharArray));
----------------
Are you sure you need the explicit `makeArrayRef` function? I thought C-arrays were implicitly convertible? If not, perhaps make them std::array, std::vector or SmallVector to reduce test noise? Applies elsewhere too.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:306
+      W.printBinary("Binary3", makeArrayRef(IntArray));
+      W.printBinary("Binary4", makeArrayRef(CharArray));
+      W.printBinaryBlock("Binary5", makeArrayRef(IntArray), 20);
----------------
I think you're missing a test case for `void printBinary(StringRef Label, StringRef Value)`


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:310
+      W.printBinaryBlock("Binary7", "FooBar");
+    }
+    return OS.str();
----------------
I think you need a test case for this if statement: https://github.com/microsoft/llvm/blob/eddbfad05ce3a85732d8bff9f85ef63afc97ad7e/lib/Support/ScopedPrinter.cpp#L25

Also, I think you should have one or more unprintable bytes that get converted to `.`. Also, I think you should at least one test case that shows what happens when there are too many bytes to fit on the line (probably interesting for both the printBinaryBlock and printBinary cases).


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

https://reviews.llvm.org/D114684