kadircet created this revision.
kadircet added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay.
Herald added a project: clang.
Currently define inline action fully qualifies any names in the
function body, which is not optimal and definitely natural.
This patch tries to improve the situation by dropping any name specifiers shared
by function and names spelled in the body. For example if you are moving
definition of a function in namespace clang::clangd, and body has any decl's
coming from clang or clang::clangd namespace, those qualifications won't be
added since they are redundant.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D69033
Files:
clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1402,6 +1402,95 @@
template <> inline void foo<int>(){})cpp")));
}
+TEST_F(DefineInlineTest, DropCommonNamespecifiers) {
+ EXPECT_EQ(apply(R"cpp(
+ namespace a { namespace b { void aux(); } }
+ namespace ns1 {
+ void foo();
+ namespace qq { void test(); }
+ namespace ns2 {
+ void bar();
+ namespace ns3 { void baz(); }
+ }
+ }
+
+ using namespace a;
+ using namespace a::b;
+ using namespace ns1::qq;
+ void ns1::ns2::ns3::b^az() {
+ foo();
+ bar();
+ baz();
+ ns1::ns2::ns3::baz();
+ aux();
+ test();
+ })cpp"),
+ R"cpp(
+ namespace a { namespace b { void aux(); } }
+ namespace ns1 {
+ void foo();
+ namespace qq { void test(); }
+ namespace ns2 {
+ void bar();
+ namespace ns3 { void baz(){
+ foo();
+ bar();
+ baz();
+ ns1::ns2::ns3::baz();
+ a::b::aux();
+ qq::test();
+ } }
+ }
+ }
+
+ using namespace a;
+ using namespace a::b;
+ using namespace ns1::qq;
+ )cpp");
+
+ EXPECT_EQ(apply(R"cpp(
+ namespace ns1 {
+ namespace qq { struct Foo { struct Bar {}; }; using B = Foo::Bar; }
+ namespace ns2 { void baz(); }
+ }
+
+ using namespace ns1::qq;
+ void ns1::ns2::b^az() { Foo f; B b; })cpp"),
+ R"cpp(
+ namespace ns1 {
+ namespace qq { struct Foo { struct Bar {}; }; using B = Foo::Bar; }
+ namespace ns2 { void baz(){ qq::Foo f; qq::B b; } }
+ }
+
+ using namespace ns1::qq;
+ )cpp");
+
+ EXPECT_EQ(apply(R"cpp(
+ namespace ns1 {
+ namespace qq {
+ template<class T> struct Foo { template <class U> struct Bar {}; };
+ template<class T, class U>
+ using B = typename Foo<T>::template Bar<U>;
+ }
+ namespace ns2 { void baz(); }
+ }
+
+ using namespace ns1::qq;
+ void ns1::ns2::b^az() { B<int, bool> b; })cpp"),
+ R"cpp(
+ namespace ns1 {
+ namespace qq {
+ template<class T> struct Foo { template <class U> struct Bar {}; };
+ template<class T, class U>
+ using B = typename Foo<T>::template Bar<U>;
+ }
+ namespace ns2 { void baz(){ qq::B<int, bool> b; } }
+ }
+
+ using namespace ns1::qq;
+ )cpp");
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -143,8 +143,48 @@
return true;
}
-// Rewrites body of FD to fully-qualify all of the decls inside.
-llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD) {
+// Gets the nested name specifiers necessary for spelling a name from
+// SourceContext in DestContext. Drops any shared nested name specifiers between
+// CurContext and SourceContext, doesn't take using declarations and directives
+// into account.
+NestedNameSpecifier *getQualification(ASTContext &Context,
+ const DeclContext *DestContext,
+ const DeclContext *SourceContext) {
+ // Goes over all parents of SourceContext until we find a comman ancestor
+ // between Dest and Source. Any qualifier including and above common ancestor
+ // is redundant, therefore we stop at lowest common ancestor.
+ std::vector<const NamespaceDecl *> SourceParents;
+ for (const DeclContext *Context = SourceContext; Context;
+ Context = Context->getLookupParent()) {
+ // Stop once we reach a common ancestor.
+ if (Context->Encloses(DestContext))
+ break;
+ // Inline namespace names are not spelled while qualifying a name, so skip
+ // those.
+ if (Context->isInlineNamespace())
+ continue;
+
+ auto *NSD = llvm::dyn_cast<NamespaceDecl>(Context);
+ assert(NSD && "Non-namespace decl context found.");
+ // Again, ananoymous namespaces are not spelled while qualifying a name.
+ if (NSD->isAnonymousNamespace())
+ continue;
+
+ SourceParents.push_back(NSD);
+ }
+
+ // Go over name-specifiers in reverse order to create necessary qualification,
+ // since we stored inner-most parent first.
+ NestedNameSpecifier *Result = nullptr;
+ for (const auto *Parent : llvm::reverse(SourceParents))
+ Result = NestedNameSpecifier::Create(Context, Result, Parent);
+ return Result;
+}
+
+// Rewrites body of FD by re-spelling all of the names to make sure they are
+// still valid in TargetContext.
+llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD,
+ const DeclContext *TargetContext) {
// There are three types of spellings that needs to be qualified in a function
// body:
// - Types: Foo -> ns::Foo
@@ -155,14 +195,13 @@
// using ns3 = ns2 -> using ns3 = ns1::ns2
//
// Go over all references inside a function body to generate replacements that
- // will fully qualify those. So that body can be moved into an arbitrary file.
+ // will qualify those. So that body can be moved into an arbitrary file.
// We perform the qualification by qualyfying the first type/decl in a
// (un)qualified name. e.g:
// namespace a { namespace b { class Bar{}; void foo(); } }
// b::Bar x; -> a::b::Bar x;
// foo(); -> a::b::foo();
- // FIXME: Instead of fully qualyfying we should try deducing visible scopes at
- // target location and generate minimal edits.
+ // FIXME: Take using declarations and directives into account.
const SourceManager &SM = FD->getASTContext().getSourceManager();
tooling::Replacements Replacements;
@@ -198,7 +237,13 @@
if (!ND->getDeclContext()->isNamespace())
return;
- std::string Qualifier = printNamespaceScope(*ND->getDeclContext());
+ std::string Qualifier;
+ if (auto *NNS = getQualification(FD->getASTContext(), TargetContext,
+ ND->getLexicalDeclContext())) {
+ llvm::raw_string_ostream OS(Qualifier);
+ NNS->print(OS, FD->getASTContext().getPrintingPolicy(), true);
+ }
+
if (auto Err = Replacements.add(
tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) {
HadErrors = true;
@@ -527,7 +572,8 @@
if (!ParamReplacements)
return ParamReplacements.takeError();
- auto QualifiedBody = qualifyAllDecls(Source);
+ auto QualifiedBody =
+ qualifyAllDecls(Source, Target->getLexicalDeclContext());
if (!QualifiedBody)
return QualifiedBody.takeError();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
