[PATCH] D101057: [clangd] Allow AST request without range

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Fantastic, thank you! Just a couple of nits.

Want me to land this for you?



================
Comment at: clang-tools-extra/clangd/DumpAST.cpp:339
+
+  bool TraverseTUDecl(TranslationUnitDecl *TU) {
+    return traverseNode("declaration", TU, [&] {
----------------
The naming causes a bit of confusion over whether this is a (CRTP) override of a RecursiveASTVisitor method. (It's not, as that method is called TraverseTranslationUnitDecl).

I'd suggest either:
 - (trivial) rename to traverseTUDecl, and move this away from the comment above that says this is overriding traversal
 - (maybe more elegant) instead of this function change the lambda inside TraverseDecl to be `if (is TUDecl) TraverseAST() else Base::TraverseDecl(D)`. Then I don't think the special case in `dumpAST()` is needed at all.


================
Comment at: clang-tools-extra/clangd/DumpAST.cpp:405
 
+// Note: It's safe for N to be a TranslationUnitDecl, as this function
+//       does not deserialize the preamble.
----------------
this comment should rather go in the .h file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101057