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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
