[PATCH] D114224: Add JSONScopedPrinter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Could you rebase this patch on top of the other commits in the series, please, so that it's easier to see the current state?



================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:580
+private:
+  // Output hex values as JSON numbers so that they're easier to parse.
+  uint64_t hexNumberToInt(HexNumber Hex) { return Hex.Value; }
----------------
I'd use the type name explicitly here. The concept of a "hex" number is purely a representation thing, so doesn't really make sense to refer to a number as a "hex number" until it's been written out somewhere. Numbers are just numbers.

I also suggest referring to the base explicitly in this comment for how they are outputted.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:591
+    for (const auto &Item : List) {
+      if (!(!Item.empty() && std::all_of(Item.begin(), Item.end(), ::isdigit)))
+        IsNumList = false;
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > I'm confused by this additional logic. It seems to me you're passing in a list of strings, but those strings might actually be numbers? That doesn't seem right to me with the calling code and/or our interface, rather than this function. For example, imagine our list happens to be a list of strings that are purely made up of digits, but the fact that they are digits is purely chance, and they should remain as strings. This function will convert to a list of integers. Further, a future new element in the same list without an all-digit input would cause the type of ALL the members of the list to change back to strings.
> Yea this makes sense. The problem I was trying to solve was that the public method `printList(...)` takes a list of template type. But to pass to the `printListImpl(...)` method currently we are using the `to_string` method. So passing a list of numbers to `printList(...)` would result in a list of strings for the `JSONScopedPrinter`.
> 
> I've reverted this change to just unconditionally print as strings for now but maybe it would be preferable to replace the template param `printList(...)` with multiple overloaded `printList(...)` methods that can delegate to `printStringListImpl(...)`/`printNumberListImpl(...)`/`printBooleanListImpl(...)` methods. Interested if you have any thoughts on this.
I think multiple functions is the way forward, although I'd just call them all `printListImpl` and let the ArrayRef type determine the version to call. Any particular reason we'd need multiple `printList` functions too? Seems like it's something that could be determined without it.

Some rough pseudo code:
```
template <typename T>
void printList(StringRef Label, const ArrayRef<T> List) {
  printListImpl(Label, List);
}

virtual void printListImpl(StringRef, ArrayRef<int> List) { /*...*/ }
virtual void printListImpl(StringRef, ArrayRef<bool> List) { /*...*/ }
virtual void printListImpl(StringRef Label, ArrayRef<std::string> List) { /*...*/ }
```

Actually, now that I type that out, I wonder if you could just get rid of `printListImpl` entirely, and make `printList` a set of virtual non-templated functions with multiple overloads for the different supported types? (Potentially you might still want a non-virtual `printListImpl` (that could be templated) to avoid code duplication of course).


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:646
+    JOS.attributeObject(Label, [&]() {
+      if (!Str.empty()) {
+        JOS.attribute("Value", Str);
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > Slightly surprised to see the new if here and a little further below. Could you explain them, please? Are you sure it's better to have attributes missing rather than as empty strings (I'm not entirely sure either way, and it probably depends on the context to some extent, but I'd focus on ease of parsing)?
> > 
> > Aside: reminder that for single line `if` and loop statements, remove the braces, i.e. here and in the for loop below.
> This was to match closer with the public methods available. Some public methods of `printBinary(...)` don't provide a `Str` param and so it's just passed into `printBinaryImpl` as `StringRef()` and the existing `ScopedPrinter::printBinaryImpl` does a similar check for `Str.empty()`. Similarly, there are public `printBinary(...)` and `printBinaryBlock(...)` methods where the latter prints the accompanying characters and the former does not. I'm not opposed to unconditionally printing these values but since these checks are used to differentiate different public methods inside `ScopedPrinter::printBinaryImpl` I thought it made sense to do the same for `JSONScopedPrinter::printBinaryImpl`.
> 
> My mistake about the `if` statments, I'll try to be more careful about it in the future.
Let's leave it as-is for now. We can always add attributes should we desire to at a future point.

That being said, I'm wondering if the `Character` block is actually useful for JSON output, since the character is just a human-readable version of the byte. If we're assuming people won't be directly reading JSON output and instead will be processing it themselves, the Character output wouldn't be useful at all, I think (they could reproduce it from the Bytes data, if needed). Thoughts?


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

https://reviews.llvm.org/D114224