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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
