[PATCH] D114224: Add JSONScopedPrinter class

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan marked 4 inline comments as done.
Jaysonyan added inline comments.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:638
+  void printHexImpl(StringRef Label, HexNumber Value) override {
+    JOS.attribute(Label, "0x" + to_hexString(Value.Value));
+  }
----------------
jhenderson wrote:
> Jaysonyan wrote:
> > jhenderson wrote:
> > > I was thinking about this, and I think we shouldn't actually print as hex in JSON, and should fallback to the standard number printing. Here's the reasoning: most of the time, we use `printHex` to print some number that represents an offset, or other value in a fixed-sized field, that is easier to read as hex. However, JSON format is not intended for human consumption, at least not in our use-case for this code. As such, readability is of lesser concern than parseability. If we were to store hex numbers as strings, rather than converting them to their decimal counterparts, we'd end up with some numeric values as strings, as others as integers, in a seemingly arbitrary manner to the end-user. I don't think that this is desirable.
> > That seems reasonable to me! Updated to output numeric values over hex strings.
> Perhaps worth a comment here elaborating on the reasoning, so that future users understand why.
I've added a comment at the top of the first method which outputs Hex. I wasn't sure if it was desirable to add a comment for every location that outputs Hex or not. Let me know if there's a preference of one over the other.


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

https://reviews.llvm.org/D114224