[PATCH] D114224: Add JSONScopedPrinter class

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan marked 9 inline comments as done.
Jaysonyan added a comment.

In D114224#3142126 <https://reviews.llvm.org/D114224#3142126>, @jhenderson wrote:

> Cross-posting this comment, since it's now applicable to this patch specifically: I think we probably need to do a bit of both: each method should be tested in isolation. I think we should ensure we have tests for the history stack, where it's applicable only. Basically, test the existing possible code paths, plus maybe one or two more interesting cases that test interaction between functions, if it seems sensible.

That seems reasonable to me! Will update with these tests soon.



================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:492
+  void printNumber(StringRef Label, uint64_t Value) override {
+    JOS.attribute(Label, to_string(Value));
+  }
----------------
jhenderson wrote:
> How come we need to stringify the value here? That seems surprising to me, and may lead to undesirable behaviour in my mind (e.g. a field for an ELF struct might be a number for the 32-bit ELF format, but a string in the 64-bit ELF format (and not all numbers will be strings in the latter-case, where they are not 64-bits)).
> 
> I have this vague memory that the LLVM JSON format doesn't support 64-bit numbers. If so, I think this is a mistake and should be fixed.
I had believed LLVM JSON originally didn't support `uint64_t` types but since then I see that support was added here: https://github.com/llvm/llvm-project/commit/8c3adce81dc36306ba30cda0cdf458cfcf7d076c so I think I can just remove the `to_string(...)`


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:524-526
+    SmallString<64> StringValue;
+    Value.toString(StringValue);
+    JOS.attribute(Label, StringValue);
----------------
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


================
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:
> 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.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:659
+                       bool Block, uint32_t StartOffset = 0) override {
+    // TODO: Implement
+  }
----------------
jhenderson wrote:
> Why's this being left as a todo?
My mistake, this was meant to be implemented before being put up for review. 


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

https://reviews.llvm.org/D114224