[PATCH] D114224: Add JSONScopedPrinter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:627
+        uint32_t CurrentOffset = StartOffset;
+        for (auto Val : Value) {
+          JOS.attribute(to_string(CurrentOffset++), Val);
----------------
Here and in the other loop below, I'd avoid the use of `auto`: it doesn't improve readability, and we tend to avoid the "always use auto" approach in LLVM, when the type isn't obvious from that line.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:648-652
+    if (ScopeCtx.Context == Scope::Object) {
+      JOS.objectBegin();
+    } else if (ScopeCtx.Context == Scope::Array) {
+      JOS.arrayBegin();
+    }
----------------



================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:524-526
+    SmallString<64> StringValue;
+    Value.toString(StringValue);
+    JOS.attribute(Label, StringValue);
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > Similar point to the above, but slightly different: I think we should compare this to `std::numeric_limits<uint64_t>::max()` and print as a number, if possible, rather than as a string.
> I updated this to just print the full `APSInt` unconditionally. Would it be preferable to cap printing numbers at `std::numeric_limits<uint64_t>::max()` and then fallback on strings otherwise? Since json doesn't have a max numerical value but arbitrarily long values may not always be supported. https://stackoverflow.com/a/39681707
Let's leave it without the cap, if we have a way of sensibly handling it. I think changing the type based on the value could get very confusing, and since as you say JSON has no firm limitations, printing out arbitrarily large numbers seems sensible.

An alternative (and probably better) approach would be to add support for this type to the JSON library directly (in a precursor patch).


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:638
+  void printHexImpl(StringRef Label, HexNumber Value) override {
+    JOS.attribute(Label, "0x" + to_hexString(Value.Value));
+  }
----------------
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.


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

https://reviews.llvm.org/D114224