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

[zhou via Phabricator via cfe-commits]

chouzz created this revision.
chouzz added reviewers: kadircet, sammccall.
Herald added a subscriber: arphaman.
Herald added a project: All.
chouzz requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This patch provide fixes about addUsing tweak doesn't traverse same-level anon 
namespaces.

Fixes: https://github.com/clangd/clangd/issues/1572


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156441

Files:
  clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
  clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp


Index: clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
@@ -395,6 +395,28 @@
 
 cc C;
 })cpp"},
+  // Should drop the qualifier in anon namespaces
+  {R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { ns::^Foo *f; ns::Bar *b;
+})cpp",
+   R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { Foo *f; ns::Bar *b;
+})cpp"},
+  // Should insert the using decl into the anon namespace
+  {R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { ns::Foo *f; ns::^Bar *b; }
+)cpp",
+   R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Bar;using ns::Foo; }
+void foo() { ns::Foo *f; Bar *b; }
+)cpp"},
   // Type defined in main file, make sure using is after that.
   {R"cpp(
 namespace xx {
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -90,12 +90,19 @@
 if (SM.getFileID(Loc) != SM.getMainFileID()) {
   return true;
 }
-if (D->getDeclContext()->Encloses(SelectionDeclContext)) {
-  Results.push_back(D);
-}
+Results.push_back(D);
 return true;
   }
 
+  bool TraverseNamespaceDecl(NamespaceDecl *D) {
+for (auto *Decl : D->decls()) {
+  if (auto *Node = dyn_cast(Decl)) {
+VisitUsingDecl(Node);
+  }
+}
+return RecursiveASTVisitor::TraverseNamespaceDecl(D);
+  }
+
   bool TraverseDecl(Decl *Node) {
 if (!Node)
   return true;


Index: clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp
@@ -395,6 +395,28 @@
 
 cc C;
 })cpp"},
+  // Should drop the qualifier in anon namespaces
+  {R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { ns::^Foo *f; ns::Bar *b;
+})cpp",
+   R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { Foo *f; ns::Bar *b;
+})cpp"},
+  // Should insert the using decl into the anon namespace
+  {R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { ns::Foo *f; ns::^Bar *b; }
+)cpp",
+   R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Bar;using ns::Foo; }
+void foo() { ns::Foo *f; Bar *b; }
+)cpp"},
   // Type defined in main file, make sure using is after that.
   {R"cpp(
 namespace xx {
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -90,12 +90,19 @@
 if (SM.getFileID(Loc) != SM.getMainFileID()) {
   return true;
 }
-if (D->getDeclContext()->Encloses(SelectionDeclContext)) {
-  Results.push_back(D);
-}
+Results.push_back(D);
 return true;
   }
 
+  bool TraverseNamespaceDecl(NamespaceDecl *D) {
+for (auto *Decl : D->decls()) {
+  if (auto *Node = dyn_cast(Decl)) {
+VisitUsingDecl(Node);
+  }
+}
+return RecursiveASTVisitor::TraverseNamespaceDecl(D);
+  }
+
   bool TraverseDecl(Decl *Node) {
 if (!Node)
   return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[zhou via Phabricator via cfe-commits]

chouzz added a comment.

@kadircet Could you please take a look this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156441

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits