hokein 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:
> > 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.
OK, let's try to make it work perfectly.
================
Comment at: change-namespace/ChangeNamespace.cpp:270
+ auto Pos = StringRef(FullOldNs).rfind(':');
+ // Ignore leading "::".
+ if (Pos != StringRef::npos && Pos > 1)
----------------
leading? looks like you are removing trailing `;`
================
Comment at: change-namespace/ChangeNamespace.cpp:277
+ auto IsVisibleInOldNs =
+ anyOf(IsInMovedNs, unless(hasAncestor(namespaceDecl(hasName(Prefix)))));
+ // Match using declarations.
----------------
Ignoring using-decls in `Prefix` namespace-decl doesn't work perfectly. The
same example:
```
namespace a { void f(); }
namespace b {
using a::f;
namespace c {
void d() { f(); }
}
}
```
When changing `b::c` to `b::e`, the `using a::f;` will be excluded by this
filter. As a result, `a::` will be added to `f()`.
https://reviews.llvm.org/D25771
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
