[PATCH] D114684: Add ScopedPrinter unit tests

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added inline comments.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:26
+
+  void verifyScopedPrinter(StringRef Expected, PrintFunc Func) {
+    Func(Writer);
----------------
jhenderson wrote:
> I'm liking the simplicity we're heading towards here, with this function and the members. As a next step, how about you add a "verifyAll" function which takes the expected strings (one for each type) and then calls the corresponding `verify*` function. This has the advantage of removing additional function calls from each test. The tests then just need to define the two strings and call the "verifyAll" function.
> 
> You could even go as far as have the fixture store StringRef members (default to empty) for `ExpectedScopedPrinter`, `ExpectedJSON` etc, and have the tests set those, rather than pass them into the function, although I'm not sure there's a huge amount of benefit to that.
Added `verifyAll` in D114224! I'll leave the expected output vars as parameters for now since both approaches seem relatively equivalent. 


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:39
+    W.printString("|");
+    W.flush();
+  };
----------------
jhenderson wrote:
> I'm in two minds about this following suggestion, so feel free to take it or leave it.
> 
> `W.flush()` is in every single lambda that I bothered checking. If we moved it to the verify* function(s), we'd avoid the need for it to be repeated here. The downside is of course that both verify functions will need it, but I think that's better than the repetition. What do you think?
That seems reasonable to me! Added.


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

https://reviews.llvm.org/D114684