This is an archive of the discontinued LLVM Phabricator instance.

[change-namespace] shorten namespace qualifier based on UsingDecl and UsingDirectiveDecl.
ClosedPublic

Authored by ioeric on Oct 19 2016, 7:39 AM.

Details

Summary

when replacing symbol references in moved namespaces, trying to make the replace
name as short as possible by considering UsingDecl (i.e. UsingShadow) and
UsingDirectiveDecl (i.e. using namespace decl).

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric updated this revision to Diff 75150.Oct 19 2016, 7:39 AM
ioeric retitled this revision from to [change-namespace] shorten namespace qualifier based on UsingDecl and UsingDirectiveDecl..
ioeric updated this object.
ioeric added a reviewer: hokein.
ioeric added a subscriber: cfe-commits.
hokein added inline comments.Oct 20 2016, 1:31 AM
change-namespace/ChangeNamespace.cpp
566 ↗(On Diff #75150)

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.

ioeric added inline comments.Oct 20 2016, 2:33 AM
change-namespace/ChangeNamespace.cpp
566 ↗(On Diff #75150)

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.

ioeric added inline comments.Oct 20 2016, 2:35 AM
change-namespace/ChangeNamespace.cpp
566 ↗(On Diff #75150)

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.

ioeric updated this revision to Diff 75280.Oct 20 2016, 3:39 AM
  • Ignore using decls in old ns but not the inner-most old ns.

Still missing one case... working on a fix.

ioeric updated this revision to Diff 75281.Oct 20 2016, 3:58 AM
  • Add a missing test case.
ioeric marked 3 inline comments as done.Oct 20 2016, 3:59 AM
ioeric added inline comments.
change-namespace/ChangeNamespace.cpp
566 ↗(On Diff #75150)

This should be fixed with the new matcher.

ioeric marked an inline comment as done.Nov 7 2016, 10:13 AM

ping.

hokein added inline comments.Nov 7 2016, 11:33 AM
change-namespace/ChangeNamespace.cpp
270 ↗(On Diff #75281)

leading? looks like you are removing trailing ;

277 ↗(On Diff #75281)

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().

566 ↗(On Diff #75150)

OK, let's try to make it work perfectly.

ioeric updated this revision to Diff 77114.Nov 7 2016, 4:15 PM
ioeric marked 2 inline comments as done.
  • Ignore using decls in old ns but not the inner-most old ns.
  • Add a missing test case.
  • Addressed comments
change-namespace/ChangeNamespace.cpp
277 ↗(On Diff #75281)

Nice catch! Fixed and explained in the new code.

ioeric updated this revision to Diff 77115.Nov 7 2016, 4:17 PM
  • Get rid of redundant PrefixRef
hokein added inline comments.Nov 8 2016, 11:06 AM
change-namespace/ChangeNamespace.cpp
233 ↗(On Diff #77115)

I will create two variables for SM.getSpellingLoc(D->getLocation()) and SM.getSpellingLoc(Loc) to avoid redundant function call.

569 ↗(On Diff #77115)

Shouldn't we check whether the using namespace decl is visible here?

ioeric updated this revision to Diff 77228.Nov 8 2016, 11:58 AM
ioeric marked 2 inline comments as done.
  • Addressed comments.
hokein edited edge metadata.Nov 8 2016, 12:41 PM

One more comment, otherwise looks good.

change-namespace/ChangeNamespace.cpp
275 ↗(On Diff #77228)

Using an invalid name - is not an elegant solution to me. Is it possible to avoid it?
Maybe we can explicitly specify IsVisibleInNewNs using the code like:

Optional<ast_matchers::internal::Matcher<NamedDecl>> IsVisibleInNewNs = IsInMovedNs;
if (!DiffOldNsSplitted.empty() )  {
  std::string Prefix = ... 
  IsVisibleInNewNs = anyOf(*IsVisibleInNewNs, unless(hasAncestor(namespaceDecl(hasName(Prefix));
}
ioeric marked an inline comment as done.Nov 8 2016, 2:24 PM
ioeric added inline comments.
change-namespace/ChangeNamespace.cpp
275 ↗(On Diff #77228)

As per offline discussion, this seems to be impossible.

hokein accepted this revision.Nov 8 2016, 2:47 PM
hokein edited edge metadata.

LGTM with one nit.

change-namespace/ChangeNamespace.cpp
275 ↗(On Diff #77228)

OK, then add a comment explicitly specifying that "-" is used as an invalid name.

I think the code can be simplified as:

std::string Prefix = "-";
if (!DiffOldNsSplitted.empty()) {
  Prefix =  (StringRef(FullOldNs).drop_back(DiffOldNamespace.size()) + DiffOldNsSplitted.front()).str();
}
This revision is now accepted and ready to land.Nov 8 2016, 2:47 PM
ioeric updated this revision to Diff 77270.Nov 8 2016, 2:51 PM
ioeric marked an inline comment as done.
ioeric edited edge metadata.
  • Addressed comment.
This revision was automatically updated to reflect the committed changes.