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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:795
+        unsigned Start, End;
+        if (auto O = positionToOffset(Inputs->Inputs.Contents, R.start))
+          Start = *O;
----------------
Please rename O to anything else. *O below looks very much like *0 and it had me really confused.


================
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>>);
+
----------------
Any reason not to name arguments here?


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


================
Comment at: clang-tools-extra/clangd/DumpAST.cpp:393
+  // DynTypedNode only works with const, RecursiveASTVisitor only non-const :-(
+  if (auto *D = N.get<Decl>())
+    V.TraverseDecl(const_cast<Decl *>(D));
----------------
Lots of lint warnings here about "const". Please fix.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1583
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
----------------
We should really disable readability-identifier-naming check on this file, it's complaining on every single identifier.


================
Comment at: clang-tools-extra/clangd/unittests/DumpASTTests.cpp:104
+  };
+  for (const auto &Case : Cases) {
+    ParsedAST AST = TestTU::withCode(Case.first).build();
----------------
Can you add a test case for class with overloaded operator(s)? They tend to cause issues here and there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89571