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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a subscriber: lebedev.ri.
riccibruno added a comment.

A few comments/questions:

1. How stable is the format going to be, and how much state is going to be exposed ?
2. I am wondering if it would be possible to separate the logic about how to visit a given AST node, from the logic about what to do with the node's state. For example it seems to me that `JSONNodeDumper::VisitVarDecl` and `TextNodeDumper::VisitVarDecl` are somewhat similar. You could instead imagine having a generic way to visit each node, which would then inform `JSONNodeDumper` or `TextNodeDumper` about the various bits of state in the node. With the proposed implementation essentially all of the logic for what is in each node has to be duplicated. This is not an objection about the current patch, just something I am wondering about.



================
Comment at: include/clang/AST/JSONNodeDumper.h:55
+  }
+
+  /// Add a child of the current node with an optional label.
----------------
Perhaps you should perfect-forward `DoAddChild` ?


================
Comment at: include/clang/AST/JSONNodeDumper.h:164
+  std::string createAccessSpecifier(AccessSpecifier AS);
+
+  void writePreviousDeclImpl(...) {}
----------------
@lebedev.ri might want to comment on the use of `llvm::json`. I think that there has been issues in the recent past where it was unexpectedly slow (although it might not matter for this use case).


================
Comment at: lib/AST/ASTDumper.cpp:231
+
+  if (ADOF_JSON == Format) {
+    JSONDumper P(OS, SM, Ctx.getPrintingPolicy());
----------------
(Ah, Yoda conditionals ! (I am not saying to change it))

Do you plan to do the same modification to the others `dump` methods ?


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

https://reviews.llvm.org/D60910