[PATCH] D114224: Add JSONScopedPrinter class

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added inline comments.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:183
   template <typename T> void printFlags(StringRef Label, T Value) {
-    startLine() << Label << " [ (" << hex(Value) << ")\n";
+    SmallVector<FlagEntry, 10> SetFlags;
     uint64_t Flag = 1;
----------------
jhenderson wrote:
> Seems like the change in this and the `printObject` function below should be part of the `ScopedPrinter` virtualisation patch?
Moved to virtualization patch!


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:386
+      raw_ostream &OS = startLine();
+      if (!Flag.Name.empty())
+        OS << "  " << Flag.Name << " (" << hex(Flag.Value) << ")\n";
----------------
jhenderson wrote:
> This check seems to be a behaviour change? Seems like we should keep that out of this patch, to keep this patch purely NFC for the regular `ScopedPrinter`
This was meant to accommodate the change I made to the making the extra `printFlags` method virtual. I was hoping both public `printFlags(...)` methods could utilize the same `printFlagsImpl` method but you're right this could be a change in behaviour. I've updated the virtualisation patch to keep this implementation the same and added an explicit `printRawFlagsImpl` method. 


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:578-581
+          if (!Flag.Name.empty())
+            JOS.value(Flag.Name);
+          else
+            JOS.value(Flag.Value);
----------------
jhenderson wrote:
> If I'm not mistaken, this is going to lead to an array of mixed types (some strings and some integers). I'm not convinced that's desirable, since it will mean needing to switch on type to determine how to handle the field. Perhaps we should do what we do elsewhere and have an array of objects, where the object has name (possibly optional) and value fields?
> 
> Alternatively, maybe the actual name isn't even needed, and the interesting thing is just the individual flag values. I guess that depends on how human readable we want this JSON output to be.
This was an attempt similar to the change in the ScopedPrinter implementation of `printFlagsImpl` to handle 2 types of public `printFlags` methods. I've added a `printRawFlagsImpl` to handle printing `Flag.Value` and updated this method to printing both `Flag.Name` and `Flag.Value`. This seems to best match the information that the `ScopedPrinter` implementation provides. 


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


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:646
+    JOS.attributeObject(Label, [&]() {
+      if (!Str.empty()) {
+        JOS.attribute("Value", Str);
----------------
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.


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:18
+
+TEST(ScopedPrinterTest, PrintIndent) {
+  auto IndentOutput = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
jhenderson wrote:
> For new unit tests that are not to do with the JSONScopedPrinter class, I'd put them in their own patch, as they are independently useful.
Moved `ScopedPrinter`-related tests to D114684 and rebased off that patch. 


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:19
+TEST(ScopedPrinterTest, PrintIndent) {
+  auto IndentOutput = [](raw_string_ostream &OS, ScopedPrinter &W) {
+    W.indent();
----------------
jhenderson wrote:
> What's the point of the lambda? It seems like it's adding unnecessary complexity to the test, when you could just do:
> ```
> std::string ScopedString;
> llvm::raw_string_ostream OS(ScopedString);
> ScopedPrinter Writer(OS);
> 
> // <current body of the lambda>
> 
> const char *Out = /*...*/;
> EXPECT_EQ(Out, ScopedString);
> ```
I haven't removed the lambda but it's now using a test fixture which takes a lambda. It doesn't need to use the test fixture since we're not testing the `JSONScopedPrinter` but I'd imagine it might be nice for the sake of consistency with other tests. Let me know if you feel otherwise. 


================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:503
+    "Bytes": {
+      "0": 70,
+      "1": 111,
----------------
jhenderson wrote:
> Looking at this test seesm to make it clear to me that this is not hte optimal format of this output. I think you probably want it to be an array of numbers, rather than an object, with individual bytes labelled.
I've updated the output to have the following format:
```
"Bytes": [
  {
    "Index": 0,
    "Value": 70,
    "Character": "F"
  },
  {
    "Index": 2,
    "Value": 111,
    "Character": "o"
  },
  ...
]
```
(Character attribute will be omitted for non `printBinaryBlock(...)` methods for now)


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

https://reviews.llvm.org/D114224