[PATCH] D60910: [WIP] Dumping the AST to JSON

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/DeclBase.h:1139
+  void dump(raw_ostream &Out, bool Deserialize = false,
+            ASTDumpOutputFormat OutputFormat = ADOF_Default) const;
 
----------------
steveire wrote:
> I think we've talked about this before, but I don't think growing interfaces like this is the best way forward. An enum is a less-good replacement for an object (ie making the user of the API responsible for creating the dumper they want to use).
> 
> I think that could be made more convenient in the future. What do you think?
I think that may be a good future improvement, yes.

One thing to take on balance when considering this for the future is that `dump()` can be called from within a debugger and that's a bit harder to accomplish with an object interface. I'm not certain that will be a big issue for my use case, but it may matter to some folks.


================
Comment at: include/clang/AST/JSONNodeDumper.h:55
+  }
+
+  /// Add a child of the current node with an optional label.
----------------
riccibruno wrote:
> Perhaps you should perfect-forward `DoAddChild` ?
Hmm, if I was doing that, I'd probably prefer to use `std::invoke()` to call the function, but we can't use that because it's C++17. Do you have concerns if I don't make that change just yet (we don't do it from `TextNodeDumper` either).


================
Comment at: include/clang/AST/JSONNodeDumper.h:53
+  template <typename Fn> void AddChild(Fn DoAddChild) {
+    return AddChild("inner", DoAddChild);
+  }
----------------
steveire wrote:
> You have 
> 
>     !Label.empty() ? Label : "inner";
> 
> below. This looks needlessly non-DRY?
There are calls to `AddChild()` from the AST traverser that pass in an empty string, which is how I got to the state I'm in (see `ASTNodeTraverser::Visit(const Stmt *, StringRef)`). I can pull the explicit label from this call and just pass an empty string.


================
Comment at: include/clang/AST/JSONNodeDumper.h:130
+// JSON and then dump to a file because the AST is an ordered collection of
+// nodes but our JSON objects are modelled as an unordered container. We can
+// use JSON facilities for constructing the terminal fields in a node because
----------------
steveire wrote:
> I'm no expert in JSON, so I'm not certain if this has implications.
> 
> Do entries in a JSON object (ie not an Array-type) have order? Are you 'giving' them order here which is not part of how JSON is usually parsed (eg by third party tools)?
> 
> Can you create ordered JSON (arrays) where it is needed? I see that `inner` is already an array, so I'm probably missing the part that is 'modelled as an unordered container'. Can you point that out to me?
Oh, good catch! I need to update those comments because they got stale.

> Can you create ordered JSON (arrays) where it is needed? I see that inner is already an array, so I'm probably missing the part that is 'modelled as an unordered container'. Can you point that out to me?

It's the key/value pair bits that are unordered, and that turns out to not matter here. I wrote that comment during a very early pass when I was passing around JSON objects more liberally and I had run into a situation where I was using kv pairs but order mattered (I don't even recall what the case was off the top of my head now). With the new design, these comments are stale -- I've corrected that.


================
Comment at: include/clang/AST/JSONNodeDumper.h:161
+      OS << ",\n";
+    OS << Indentation << "\"" << Key << "\": {\n";
+    Indentation += "  ";
----------------
steveire wrote:
> Is it possible this 'textual' JSON generation could produce invalid JSON? One of the things a library solution ensures is balanced braces and valid syntax etc. I'm not familiar enough with JSON to know if it has dark corners.
I don't believe this will produce invalid JSON, short of aborting mid-stream.


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

https://reviews.llvm.org/D60910