[clang-tools-extra] 5075c68 - [clangd] Improve symbol qualification in DefineInline code action
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 25 01:42:24 PST 2019
Author: Kadir Cetinkaya
Date: 2019-11-25T10:42:13+01:00
New Revision: 5075c68219826a199c67f7450c7cf60a55a71c0b
URL: https://github.com/llvm/llvm-project/commit/5075c68219826a199c67f7450c7cf60a55a71c0b
DIFF: https://github.com/llvm/llvm-project/commit/5075c68219826a199c67f7450c7cf60a55a71c0b.diff
LOG: [clangd] Improve symbol qualification in DefineInline code action
Summary:
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.
It also drops any qualifiers that are visible in target context.
Reviewers: ilya-biryukov
Subscribers: MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D69033
Added:
Modified:
clang-tools-extra/clangd/AST.h
clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h
index bea67a41005a..bc38f19c6553 100644
--- a/clang-tools-extra/clangd/AST.h
+++ b/clang-tools-extra/clangd/AST.h
@@ -137,6 +137,8 @@ llvm::Optional<QualType> getDeducedType(ASTContext &, SourceLocation Loc);
/// 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
index 6d0599e8821c..3bc5df0edbfd 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -135,8 +135,10 @@ bool checkDeclsAreVisible(const llvm::DenseSet<const Decl *> &DeclRefs,
return true;
}
-// Rewrites body of FD to fully-qualify all of the decls inside.
-llvm::Expected<std::string> 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<std::string> 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 @@ llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD) {
// 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 @@ llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD) {
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 @@ class DefineInline : public Tweak {
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
index ab2808835832..4e481241acd8 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -1667,6 +1667,148 @@ TEST_F(DefineInlineTest, HandleMacros) {
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<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"},
+ };
+ 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
More information about the cfe-commits
mailing list