[PATCH] D89571: [clangd] Add textDocument/ast extension method to dump the AST

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 4 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:323
+  /// Describe the AST subtree for a piece of code.
+  void getAST(PathRef File, Range, Callback<llvm::Optional<ASTNode>>);
+
----------------
adamcz wrote:
> Any reason not to name arguments here?
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?)


================
Comment at: clang-tools-extra/clangd/DumpAST.cpp:241
+      return MTE->isBoundToLvalueReference() ? "lvalue" : "rvalue";
+    return "";
+  }
----------------
adamcz wrote:
> Maybe instead of "" the fallback should be to just return the code in the getSourceRange() range?
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


================
Comment at: clang-tools-extra/clangd/Protocol.h:1583
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
----------------
adamcz wrote:
> We should really disable readability-identifier-naming check on this file, it's complaining on every single identifier.
Agree - but I don't actually know how to do this without turning it off for the whole directory. (We could do that...)


================
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:104
+  };
+  for (const auto &Case : Cases) {
+    ParsedAST AST = TestTU::withCode(Case.first).build();
----------------
adamcz wrote:
> Can you add a test case for class with overloaded operator(s)? They tend to cause issues here and there.
Added one with use of an overloaded operator (LMK if you wanted to dump the declaration instead)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89571