[PATCH] D25771: [change-namespace] shorten namespace qualifier based on UsingDecl and UsingDirectiveDecl.

[Eric Liu via cfe-commits]

ioeric marked 3 inline comments as done.
ioeric added inline comments.


================
Comment at: change-namespace/ChangeNamespace.cpp:566
+      break;
+    if (isDeclVisibleAtLocation(*Result.SourceManager, Using, DeclCtx, Start)) {
+      for (const auto *UsingShadow : Using->shadows()) {
----------------
ioeric wrote:
> ioeric wrote:
> > hokein wrote:
> > > Yeah, it works for most cases, but it can not guarantee that they are still in the same DeclContext after changing to new namespace.
> > > 
> > > For example, the following case where an using-decl is used in the namespace being renamed, we change `b::c` to `d::e`, although DeclContext `d` of callexpr `f()` is a nested DeclContext of `b` (also DeclContext of `using a::f`), but this assumption is wrong after changing namespace because we keep `using a::f` in its original namespace.
> > > 
> > > ```
> > > namespace a { void f(); }
> > > 
> > > namespace b {
> > > using a::f;
> > > namespace c {
> > > void d() { f(); }
> > > }  // c
> > > } // b
> > > ```
> > > 
> > > Not sure whether we should do this in our tool. I suspect there might be a lot of corner cases.
> > > 
> > I thought using decls in old namespaces should be moved with along with namespaces. If this is the case, the moving of using decls is unexpected (looking into this), but this patch is handling this correctly if using decls are not moved.
> Ahh, I was wrong. `using a::f` should not be moved. Hmm, I think we can solve or at least workaround this. But I still think we should support shortening namespace specifier based on using decls; it is quite not nice to add long specifiers if there are already using decls present.
This should be fixed with the new matcher.


https://reviews.llvm.org/D25771