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

[Haojian Wu via cfe-commits]

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