[PATCH] D114224: Add JSONScopedPrinter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You should add tests (for both classes) for `classof` and `getKind` functions, since they're part of the public API.

One thought that's occurred to me: should the default JSON output really be formatted so verbosely (i.e. with all the new lines and spacing)? As the prime motivation of this is for machine readability, would it not make more sense for it to be compact, at least by default (with a possible option for pretty printing)? The amount of whitespace that's in the output could make the output data size be significantly larger than it needs to be, slowing down the parsing of said output.



================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:96
 public:
-  ScopedPrinter(raw_ostream &OS) : OS(OS), IndentLevel(0) {}
+  enum ScopedPrinterKind {
+    Standard,
----------------
All the new enums probably want to be `enum class`.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:97
+  enum ScopedPrinterKind {
+    Standard,
+    JSON,
----------------
I'd avoid the use of the name `Standard`, as it could be confused with the C++ standard. I'd use `Base`, `Plain` or `Basic` for the enum value.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:509
+  enum ScopeKind {
+    Standard,
+    Attribute,
----------------
Similar comment to the above: don't use the name "Standard" for an enum value. Actually, in this case, without looking at the code, I have no clue what "Standard" even means. It should be renamed to reflect what it is used for.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:521
+
+  SmallVector<ScopeContext, 16> ScopeHistory;
+  json::OStream JOS;
----------------
At one point, there was some effort to set good defaults for SmallVector, allowing you to omit the second argument. It's less useful here, because we have a good guess about the amount of nesting. I think it's more likely to be a max of about 4 or 5, so 16 is probably way too high. Perhaps worth doing an audit of the usage within the LLVM code and add a few more points above the current maximum scoping (yes, I know JSONScopedPrinter isn't used really yet, but you can imagine how it would look, based on how ScopedPrinter is used already).


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:1063
 
 TEST_F(ScopedPrinterTest, StartLine) {
   auto PrintFunc = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
I think we need a JSONScopedPrinter version of this test, since we override `startLine` in that class.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:1077
 
 TEST_F(ScopedPrinterTest, GetOStream) {
   auto PrintFunc = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
Ditto.


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

https://reviews.llvm.org/D114224