[PATCH] D65101: [clangd] SelectionTree treats TranslationUnitDecl consistently with other containers.

[Sam McCall via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp:47
   // Narrow the traversal scope to the selected node.
   Inputs.AST.getASTContext().setTraversalScope(
       {const_cast<Decl *>(InterestedDecl)});
----------------
hokein wrote:
> we'll get a TUDecl if the whole file is selected, however this tweak doesn't want to traversal the whole TU (otherwise we will generate highlighting tokens for #included files), we only interested in the top level decls of the main file. 
Right! We discussed this further offline, and came to the conclusions that:
 - traversing the whole TU is undesirable almost always
 - it's already possible to get the whole TU from commonAncestor today (by selecting multiple top-level decls), and there are resulting bugs
 - returning null instead of TU from commonAncestor() is a safer default


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DumpAST.cpp:38
          N = N->Parent)
-      if (dumpable(N->ASTNode))
+      if (N->Parent && dumpable(N->ASTNode))
         Node = N->ASTNode;
----------------
hokein wrote:
> Is the `N->Parent` intended?  this seems like we'd exclude the TUDecl?
This is obsolete now, but was useful to flag: it's exactly the type of question that tweaks shouldn't have to worry about by default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65101