[PATCH] D69162: [clangd] Remove using-namespace present inside a namespace.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The most important comment is in the tests. Is there a way to have the same effect with less changes?



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:47
   const UsingDirectiveDecl *TargetDirective = nullptr;
+  const NamespaceDecl *ContainingNS = nullptr;
 };
----------------
NIT: could you add a short comment mentioning this is exactly the `DeclContext` of `TargetDirective`?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:110
+  auto DeclCtx = TargetDirective->getDeclContext();
+  if (!dyn_cast<Decl>(DeclCtx))
     return false;
----------------
This check looks redundant now that we check the decl context is a namespace of a translation unit.
I think it was originally intended to check the whether the `using namespace` is inside a function or not.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:173
+
+    CharSourceRange RemoveQualifier;
+    RemoveQualifier.setBegin(Ref.Qualifier ? Ref.Qualifier.getBeginLoc() : Loc);
----------------
NIT: maybe simplify to
```
QualifierReplacements.push_back(
  CharSourceRange::getTokenRange(
    Ref.Qualifier ? Ref.Qualifier.getBeginLoc : Loc, 
    Loc));
```
?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:180
+  if (ContainingNS)
+    findExplicitReferences(ContainingNS, SelectRefToQualify);
+  else
----------------
Do we also need to run in the redeclarations of the same namespace?
```
namespace clangd {
  using namespace clang;
}

namespace clangd {
  /*clang::*/Decl* D = nullptr;
}
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:182
+  else
+    for (auto &D : Inputs.AST.getLocalTopLevelDecls())
+      findExplicitReferences(D, SelectRefToQualify);
----------------
NIT: could you add braces around the branches of the if statement?
for loop is complicated, so it probably qualifies as something that asks for the braces.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:898
+        A::B::vector V3;
+        A::B::std::vector V4;
+        ::A::B::std::vector V5;
----------------
Is there a way to avoid re-qualifying this?
We should aim for minimal changes and there is probably a simple way to achieve this by only qualifying when the qualifier itself references the `ContainingNS` and target is inside the target namespace of the directive being removed.

Or this too complicated for some reason?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69162