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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz accepted this revision.
adamcz added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:799
+          return CB(Offset.takeError());
+        if (auto O = positionToOffset(Inputs->Inputs.Contents, R.end))
+          End = *O;
----------------
Also here please ;)


================
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>>);
+
----------------
sammccall wrote:
> 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?)
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.


================
Comment at: clang-tools-extra/clangd/DumpAST.cpp:209
+  std::string getDetail(const Decl *D) {
+    auto *ND = dyn_cast<NamedDecl>(D);
+    if (!ND || llvm::isa_and_nonnull<CXXConstructorDecl>(ND->getAsFunction()) ||
----------------
lint says this can be const


================
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:12
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
----------------
include order lint warning here


================
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:104
+  };
+  for (const auto &Case : Cases) {
+    ParsedAST AST = TestTU::withCode(Case.first).build();
----------------
sammccall wrote:
> 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)
Thanks, that's exactly what I wanted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89571