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:
> 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.
https://reviews.llvm.org/D25771
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
