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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
