Page MenuHomePhabricator

[clangd] Add textDocument/ast extension method to dump the AST
ClosedPublic

Authored by sammccall on Oct 16 2020, 11:05 AM.

Details

Summary

This is a mass-market version of the "dump AST" tweak we have behind
-hidden-features.
I think in this friendlier form it'll be useful for people outside clang
developers, which would justify making it a real feature.
It could be useful as a step towards lightweight clang-AST tooling in clangd
itself (like matcher-based search).

Advantages over the tweak:

  • simplified information makes it more accessible, likely somewhat useful without learning too much clang internals
  • can be shown in a tree view
  • structured information gives some options for presentation (e.g. icon + two text colors + tooltip in vscode)
  • clickable nodes jump to the corresponding code

Disadvantages:

  • a bunch of code to handle different node types
  • likely missing some important info vs dump-ast due to brevity/oversight
  • may end up chasing/maintaining support for the long tail of nodes

Demo with VSCode support: https://imgur.com/a/6gKfyIV

Diff Detail

Event Timeline

sammccall created this revision.Oct 16 2020, 11:05 AM
sammccall requested review of this revision.Oct 16 2020, 11:05 AM
sammccall updated this revision to Diff 298959.Oct 19 2020, 2:32 AM

Oops, forgot newly added files :-\

sammccall updated this revision to Diff 298961.Oct 19 2020, 2:33 AM

(for real this time)

adamcz added inline comments.Oct 26 2020, 10:46 AM
clang-tools-extra/clangd/ClangdServer.cpp
795

Please rename O to anything else. *O below looks very much like *0 and it had me really confused.

clang-tools-extra/clangd/ClangdServer.h
323

Any reason not to name arguments here?

clang-tools-extra/clangd/DumpAST.cpp
242

Maybe instead of "" the fallback should be to just return the code in the getSourceRange() range?

394

Lots of lint warnings here about "const". Please fix.

clang-tools-extra/clangd/Protocol.h
1672

We should really disable readability-identifier-naming check on this file, it's complaining on every single identifier.

clang-tools-extra/clangd/unittests/DumpASTTests.cpp
105

Can you add a test case for class with overloaded operator(s)? They tend to cause issues here and there.

sammccall marked 4 inline comments as done.Wed, Oct 28, 11:59 AM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdServer.h
323

I generally prefer not to name them in the declaration when it doesn't communicate anything beyond the type.

Done here as most of the other functions name them.

(I'm curious, do you think there are useful names here, or is the irregularity between named/unnamed bothersome?)

clang-tools-extra/clangd/DumpAST.cpp
242

This goes against a couple of related goals I have here:

  • the detail should be short enough to quickly scan, roughly it should be one name (a class name is OK, but an arbitrary function signature isn't)
  • the detail should relate to this *node*, not the whole subtree
clang-tools-extra/clangd/Protocol.h
1672

Agree - but I don't actually know how to do this without turning it off for the whole directory. (We could do that...)

clang-tools-extra/clangd/unittests/DumpASTTests.cpp
105

Added one with use of an overloaded operator (LMK if you wanted to dump the declaration instead)

sammccall marked 2 inline comments as done.

Address comments

adamcz accepted this revision.Thu, Oct 29, 9:31 AM
adamcz added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
799

Also here please ;)

clang-tools-extra/clangd/ClangdServer.h
323

Unnamed argument makes me think that it's ignored by this function. It's in-line with Google style guide, maybe doesn't necessary apply here.

clang-tools-extra/clangd/DumpAST.cpp
210

lint says this can be const

clang-tools-extra/clangd/unittests/DumpASTTests.cpp
13

include order lint warning here

105

Thanks, that's exactly what I wanted.

This revision is now accepted and ready to land.Thu, Oct 29, 9:31 AM
sammccall marked 3 inline comments as done.Thu, Nov 19, 4:13 PM
sammccall added inline comments.
clang-tools-extra/clangd/unittests/DumpASTTests.cpp
13

Not sure what's going on here, this does seem to be the order that current clang-format wants. Maybe this clang-tidy is an older version?

This revision was landed with ongoing or failed builds.Thu, Nov 19, 4:15 PM
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
kadircet added inline comments.Thu, Nov 19, 11:47 PM
clang-tools-extra/clangd/unittests/DumpASTTests.cpp
13