diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h --- a/clang-tools-extra/clangd/AST.h +++ b/clang-tools-extra/clangd/AST.h @@ -137,6 +137,8 @@ /// InsertionPoint. i.e, if you have `using namespace /// clang::clangd::bar`, this function will return an empty string for the /// example above since no qualification is necessary in that case. +/// FIXME: Also take using directives and namespace aliases inside function body +/// into account. std::string getQualification(ASTContext &Context, const DeclContext *DestContext, SourceLocation InsertionPoint, diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp --- a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp @@ -135,8 +135,10 @@ return true; } -// Rewrites body of FD to fully-qualify all of the decls inside. -llvm::Expected qualifyAllDecls(const FunctionDecl *FD) { +// Rewrites body of FD by re-spelling all of the names to make sure they are +// still valid in context of Target. +llvm::Expected qualifyAllDecls(const FunctionDecl *FD, + const FunctionDecl *Target) { // There are three types of spellings that needs to be qualified in a function // body: // - Types: Foo -> ns::Foo @@ -147,16 +149,16 @@ // 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. + auto *TargetContext = Target->getLexicalDeclContext(); const SourceManager &SM = FD->getASTContext().getSourceManager(); + tooling::Replacements Replacements; bool HadErrors = false; findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) { @@ -193,7 +195,8 @@ if (!ND->getDeclContext()->isNamespace()) return; - std::string Qualifier = printNamespaceScope(*ND->getDeclContext()); + const std::string Qualifier = getQualification( + FD->getASTContext(), TargetContext, Target->getBeginLoc(), ND); if (auto Err = Replacements.add( tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) { HadErrors = true; @@ -437,7 +440,7 @@ if (!ParamReplacements) return ParamReplacements.takeError(); - auto QualifiedBody = qualifyAllDecls(Source); + auto QualifiedBody = qualifyAllDecls(Source, Target); if (!QualifiedBody) return QualifiedBody.takeError(); diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -1667,6 +1667,148 @@ EXPECT_EQ(apply(Case.first), Case.second) << Case.first; } +TEST_F(DefineInlineTest, DropCommonNameSpecifiers) { + struct { + llvm::StringRef Test; + llvm::StringRef Expected; + } Cases[] = { + {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"}, + {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"}, + {R"cpp( + namespace ns1 { + namespace qq { + template struct Foo { template struct Bar {}; }; + template + using B = typename Foo::template Bar; + } + namespace ns2 { void baz(); } + } + + using namespace ns1::qq; + void ns1::ns2::b^az() { B b; })cpp", + R"cpp( + namespace ns1 { + namespace qq { + template struct Foo { template struct Bar {}; }; + template + using B = typename Foo::template Bar; + } + namespace ns2 { void baz(){ qq::B b; } } + } + + using namespace ns1::qq; + )cpp"}, + }; + for (const auto &Case : Cases) + EXPECT_EQ(apply(Case.Test), Case.Expected) << Case.Test; +} + +TEST_F(DefineInlineTest, QualifyWithUsingDirectives) { + llvm::StringRef Test = R"cpp( + namespace a { + void bar(); + namespace b { struct Foo{}; void aux(); } + namespace c { void cux(); } + } + using namespace a; + using X = b::Foo; + void foo(); + + using namespace b; + using namespace c; + void ^foo() { + cux(); + bar(); + X x; + aux(); + using namespace c; + // FIXME: The last reference to cux() in body of foo should not be + // qualified, since there is a using directive inside the function body. + cux(); + })cpp"; + llvm::StringRef Expected = R"cpp( + namespace a { + void bar(); + namespace b { struct Foo{}; void aux(); } + namespace c { void cux(); } + } + using namespace a; + using X = b::Foo; + void foo(){ + c::cux(); + bar(); + X x; + b::aux(); + using namespace c; + // FIXME: The last reference to cux() in body of foo should not be + // qualified, since there is a using directive inside the function body. + c::cux(); + } + + using namespace b; + using namespace c; + )cpp"; + EXPECT_EQ(apply(Test), Expected) << Test; +} + } // namespace } // namespace clangd } // namespace clang