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