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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:179
+
+  if (ContainingNS) {
+    for (auto ReDeclNS : ContainingNS->redecls())
----------------
usaxena95 wrote:
> ilya-biryukov wrote:
> > Could you explain why don't we always just run across the whole TU?
> > What disadvantages would that have?
> Working with a using decl inside the `ContainingNS` is quite similar to our initial version with GlobalNS. 
> For example we take advantage of that to replace the whole qualifier with just TargetNS. We use the fact that since `using namespace TargetNS;` was valid inside `ContainingNS` then qualifying the reference with just `TargetNS` would also be valid inside `ContainingNS`.
> 
> When we are outside `ContainingNS`,  we would have to fully qualify the ref to be 100% correct. To reduce the qualification we might have to figure out which is enclosing NS for the reference.
> 
> ```
> namespace aa {
> namespace bb { 
> struct map {}; 
> }
> using namespace b^b;
> }
> int main() { 
>  aa::map m1;  // We do not qualify this currently.
> }
> ```
> 
If we detect references outside `ContainingNS`, which are definitely broken, fully-qualifying seems better than silently ignoring those.

I think they're quite easy to detect:
- Their target should be inside `TargetNS`,
- Their `QualifierLoc` should reference `ContainingNS`.

We know that those won't work anymore, so we should qualify them.

Leaving broken code is definitely a bad idea, if we can easily detect and fix it.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:110
     return false;
-  if (!dyn_cast<Decl>(TargetDirective->getDeclContext()))
-    return false;
+  auto *DeclCtx = TargetDirective->getDeclContext();
   // FIXME: Unavailable for namespaces containing using-namespace decl.
----------------
NIT: you could use `DC` here, clangd uses Go-style short identifiers for locals and parameters extensively


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:146
+  std::vector<CharSourceRange> QualifierReplacements;
+  auto SelectRefToQualify = [&](ReferenceLoc Ref) {
+    if (Ref.Targets.empty())
----------------
NIT: Maybe call it `QualifyRef`? Was a bit confused by `Select`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69162