[PATCH] D114224: Add JSONScopedPrinter class

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added inline comments.


================
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;
----------------
jhenderson wrote:
> 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).
I've added overloaded methods for `printList(...)` inside the virtualization diff and updated the ScopedPrinter test diff and this diff accordingly.

The implementation roughly follows the ideas you've laid out but the major difference is that the overloaded methods are public `printList(...)` methods rather than private `printListImpl(...)`. This was needed because we still need to maintain a template `printList(...)` method to fall-back on for lists not comprised of strings, ints, or booleans. 


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:646
+    JOS.attributeObject(Label, [&]() {
+      if (!Str.empty()) {
+        JOS.attribute("Value", Str);
----------------
jhenderson wrote:
> 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?
You're right, adding the characters doesn't provide much value if we're analyzing the output through automated scripts. I think it makes sense to remove this block.


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

https://reviews.llvm.org/D114224