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