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).
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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. |
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. |
change-namespace/ChangeNamespace.cpp | ||
---|---|---|
566 ↗ | (On Diff #75150) | This should be fixed with the new matcher. |
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. |
- 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. |
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? Optional<ast_matchers::internal::Matcher<NamedDecl>> IsVisibleInNewNs = IsInMovedNs; if (!DiffOldNsSplitted.empty() ) { std::string Prefix = ... IsVisibleInNewNs = anyOf(*IsVisibleInNewNs, unless(hasAncestor(namespaceDecl(hasName(Prefix)); } |
change-namespace/ChangeNamespace.cpp | ||
---|---|---|
275 ↗ | (On Diff #77228) | As per offline discussion, this seems to be impossible. |
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(); } |