[PATCH] D68562: [clangd] Add RemoveUsingNamespace tweak.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:87
+    return false;
+  if (const Decl *ParentDecl = Node->Parent->ASTNode.get<Decl>())
+    return llvm::isa<TranslationUnitDecl>(ParentDecl);
----------------
Can we use `ASTNode.get<TranslationUnitDecl>()` to directly to check for this?

Not sure how `DynTypedNode` works, though, maybe that's impossible.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:100
+bool isInsideNamespace(const DeclContext *D, const NamespaceDecl *Target) {
+  while (D && (D->isNamespace() || D->isTransparentContext())) {
+    if (D->Equals(Target))
----------------
Hm, I'd expect us to simply go up until the first non-trasparent namespace and stop at it.
Consider the following example:
```
namespace a { namespace b {
  struct Foo {};
}}

using namespace a; // <-- remove this
using namespace b;

Foo x; // <-- Foo does not need to be qualified, as it's inside b.
```

OTOH, in case of inline namespace we'd choose to qualify:
```
namespace a { inline namespace b {
  struct Foo {};
}}

using namespace a; // <-- remove this

Foo x; // <-- need to change to a::Foo
```

Obviously, if we had `using namespace a::b` we'd not want to qualify, but that's complicated. And it's very rare to have `using namespace` for an inline namespace.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:782
+      int main() {
+        aa::map m;
+      }
----------------
This is incorrect, right? We should not be qualifying here.
See the relevant comment on `isInsideNamespace`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68562