Page MenuHomePhabricator

Allow the creation of human-friendly ASTDumper to arbitrary output stream

Authored by whisperity on Mar 30 2018, 8:57 AM.



ASTPrinter allows setting the ouput to any O-Stream, but that printer creates source-code-like syntax (and is also marked with a FIXME). The nice, colourful, mostly human-readable ASTDumper only works on the standard output, which is not feasible in case a user wants to see the AST of a file through a code navigation/comprehension tool.

This small addition of an overload solves generating a nice colourful AST block for the users of a tool I'm working on, CodeCompass, as opposed to having to duplicate the behaviour of definitions that only exist in the anonymous namespace of implementation TUs related to this module.

Diff Detail


Event Timeline

whisperity created this revision.Mar 30 2018, 8:57 AM
alexfh accepted this revision.Apr 5 2018, 6:24 AM

Looks good to me. One optional comment.

39–41 ↗(On Diff #140437)

With just three users of the old API it might make sense to just add a parameter to it and update the callers instead of adding another override.

This revision is now accepted and ready to land.Apr 5 2018, 6:24 AM
  • Overload removed, now only one CreateASTDumper function remains.
  • Updated the call sites of this function to use this call.
whisperity requested review of this revision.Apr 5 2018, 12:09 PM
whisperity marked an inline comment as done.

@alexfh I have updated the patch. I don't have commit rights, so if you think this is good to go, could you please commit for me?

alexfh added a comment.Apr 6 2018, 4:14 AM

Another couple of nits.

136 ↗(On Diff #141194)

Please add an argument comment for nullptr, e.g.

return clang::CreateASTDumper(/*OS=*/nullptr, ...);

or maybe

return clang::CreateASTDumper(nullptr /*dump to stderr*/, ...);

Same in the actual callers below.

38 ↗(On Diff #141194)

Add "When OS is nullptr, dumps AST to stderr."

whisperity edited the summary of this revision. (Show Details)Apr 6 2018, 4:22 AM
whisperity updated this revision to Diff 141309.Apr 6 2018, 4:27 AM

Added comments on what nullptr means at call sites.

whisperity marked 2 inline comments as done.Apr 6 2018, 4:28 AM

It is also std-out (llvm::outs()) in case of nullptr and not std-err.

alexfh accepted this revision.Apr 6 2018, 5:48 AM

LG. I'll commit the patch for you.

This revision is now accepted and ready to land.Apr 6 2018, 5:48 AM
This revision was automatically updated to reflect the committed changes.