[PATCH] D156441: [clangd]Fix addUsing tweak doesn't traverse same-level anon namespaces

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This would be a great fix to have! However I don't think the specific changes to the RecursiveASTVisitor are correct.

I can see a couple of approaches:

1. prevent the RecursiveASTVisitor from traversing into uninteresting contexts, and drop the Encloses check in VisitUsingDecl
2. explicitly gather the list of interesting contexts

option 1 is most similar to this patch, I believe TraverseNamespaceDecl needs to call base::TraverseNamespaceDecl only if the NS either encloses the selection DC or is anonymous.

option 2 would mean walking up the declcontext chain and grabbing each entry and also getAnonymousNamespace() from TranslationUnitDecl and NamespaceDecl, and changing our Encloses() tests to check membership in that set.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:97
 
+  bool TraverseNamespaceDecl(NamespaceDecl *D) {
+    for (auto *Decl : D->decls()) {
----------------
This doesn't look like it can be right:
 - there's no distinction on whether the namespace is anonymous or not
 - you're calling VisitUsingDecl, but also Base::TraverseNamespaceDecl which will visit the contained using decls again

this also suggests we need more testcases here: some negative ones verifying that we don't look at UsingDecls that are inside named namespaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156441