[PATCH] D114224: Add JSONScopedPrinter class

Thu Nov 25 01:02:34 PST 2021

jhenderson added a comment.

I've only skimmed the new tests, but as there are some structural issues there, I suggest you address those and then I'll confirm the test coverage and logic looks good.

================
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;
----------------
Seems like the change in this and the `printObject` function below should be part of the `ScopedPrinter` virtualisation 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";
----------------
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`

================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:578-581
+          if (!Flag.Name.empty())
+            JOS.value(Flag.Name);
+          else
+            JOS.value(Flag.Value);
----------------
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.

================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:590
+    bool IsNumList = true;
+    for (const auto &Item : List) {
+      if (!(!Item.empty() && std::all_of(Item.begin(), Item.end(), ::isdigit)))
----------------
Same below.

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

================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:603
+
+  // Output hex values as JSON number so it's easier to parse
+  void printHexListImpl(StringRef Label,
----------------
I'm inclined to say we should have the comment by each function. Perhaps better though, at the cost of a little more code than would otherwise be necessary, would be to have a small helper function `hexNumberToInt` or similar, which is then called in all places you currently do `HexNumber.Value`. You could then put the comment with that function, in one place.

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

================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:16
+
+namespace {
+
----------------
I wouldn't bother with the anonymous namespace. There'd only be a problem if someone else started writing ScopedPrinterTests in another file, which I think we'd want to spot and possibly stop.

================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:18
+
+TEST(ScopedPrinterTest, PrintIndent) {
+  auto IndentOutput = [](raw_string_ostream &OS, ScopedPrinter &W) {
----------------
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.

================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:19
+TEST(ScopedPrinterTest, PrintIndent) {
+  auto IndentOutput = [](raw_string_ostream &OS, ScopedPrinter &W) {
+    W.indent();
----------------
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);
```

================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:34
+
+  std::string ScopedString;
+  llvm::raw_string_ostream OS(ScopedString);
----------------
This string isn't really scoped itself. It's more the buffer used by the stream, so I'd just call it `Buffer` or `StreamBuffer`.

================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:35
+  std::string ScopedString;
+  llvm::raw_string_ostream OS(ScopedString);
+  ScopedPrinter Writer(OS);
----------------

================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:92-94
+  std::string ScopedString;
+  llvm::raw_string_ostream OS(ScopedString);
+  ScopedPrinter Writer(OS);
----------------
This setup logic might benefit from being pulled into a test fixture, to reduce the duplication between tests.

The JSON stuff could go in the same fixture.

================
Comment at: llvm/unittests/Support/ScopedPrinterTest.cpp:503
+    "Bytes": {
+      "0": 70,
+      "1": 111,
----------------
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.

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

https://reviews.llvm.org/D114224