[PATCH] D114224: Add JSONScopedPrinter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've not yet looked at the JSONScopedPrinter tests again, but the body of the code looks pretty good.



================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:652
+private:
+  // Output HexNumber as decimal so that they're easier to parse.
+  uint64_t hexNumberToInt(HexNumber Hex) { return Hex.Value; }
----------------
This comment applies to all HexNumber instances, not just the one passed in, so it should be plural.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:737
+          JOS.objectBegin();
+          JOS.attribute("Index", CurrentOffset++);
+          JOS.attribute("Value", Val);
----------------
I'm now looking at this `Index` field and wondering if it's useful at this position? We're in an array already, so the index should match the array position. It doesn't (so should be called Offset), but then why have an entry per byte, when one per array would be less verbose, and equally as informative? It would also allow the values to be written directly in the array rather than in a nested object. I.e.
```
{
  "Value" : <Str>,
  "Offset" : <StartOffset>,
  "Bytes" : [Value[0], Value[1], ...]
}
```


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

https://reviews.llvm.org/D114224