[clang-tools-extra] 71aa3f7 - [clangd] Add parameter renaming to define-inline code action
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 31 01:28:19 PDT 2019
Author: Kadir Cetinkaya
Date: 2019-10-31T09:23:09+01:00
New Revision: 71aa3f7b7e43bf7d2a8a38324b1f9f7b12bbbdfc
URL: https://github.com/llvm/llvm-project/commit/71aa3f7b7e43bf7d2a8a38324b1f9f7b12bbbdfc
DIFF: https://github.com/llvm/llvm-project/commit/71aa3f7b7e43bf7d2a8a38324b1f9f7b12bbbdfc.diff
LOG: [clangd] Add parameter renaming to define-inline code action
Summary:
When moving a function definition to declaration location we also need
to handle renaming of the both function and template parameters.
This patch achives that by making sure every parameter name and dependent type
in destination is renamed to their respective name in the source.
Reviewers: ilya-biryukov
Subscribers: MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D68937
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
index 6bce81bae306..f6966f619ade 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -226,6 +226,107 @@ llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD) {
return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
}
+/// Generates Replacements for changing template and function parameter names in
+/// \p Dest to be the same as in \p Source.
+llvm::Expected<tooling::Replacements>
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+ llvm::DenseMap<const Decl *, std::string> ParamToNewName;
+ llvm::DenseMap<const NamedDecl *, std::vector<SourceLocation>> RefLocs;
+ auto HandleParam = [&](const NamedDecl *DestParam,
+ const NamedDecl *SourceParam) {
+ // No need to rename if parameters already have the same name.
+ if (DestParam->getName() == SourceParam->getName())
+ return;
+ std::string NewName;
+ // Unnamed parameters won't be visited in findExplicitReferences. So add
+ // them here.
+ if (DestParam->getName().empty()) {
+ RefLocs[DestParam].push_back(DestParam->getLocation());
+ // If decl is unnamed in destination we pad the new name to avoid gluing
+ // with previous token, e.g. foo(int^) shouldn't turn into foo(intx).
+ NewName = " ";
+ }
+ NewName.append(SourceParam->getName());
+ ParamToNewName[DestParam->getCanonicalDecl()] = std::move(NewName);
+ };
+
+ // Populate mapping for template parameters.
+ auto *DestTempl = Dest->getDescribedFunctionTemplate();
+ auto *SourceTempl = Source->getDescribedFunctionTemplate();
+ assert(bool(DestTempl) == bool(SourceTempl));
+ if (DestTempl) {
+ const auto *DestTPL = DestTempl->getTemplateParameters();
+ const auto *SourceTPL = SourceTempl->getTemplateParameters();
+ assert(DestTPL->size() == SourceTPL->size());
+
+ for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I)
+ HandleParam(DestTPL->getParam(I), SourceTPL->getParam(I));
+ }
+
+ // Populate mapping for function params.
+ assert(Dest->param_size() == Source->param_size());
+ for (size_t I = 0, E = Dest->param_size(); I != E; ++I)
+ HandleParam(Dest->getParamDecl(I), Source->getParamDecl(I));
+
+ const SourceManager &SM = Dest->getASTContext().getSourceManager();
+ const LangOptions &LangOpts = Dest->getASTContext().getLangOpts();
+ // Collect other references in function signature, i.e parameter types and
+ // default arguments.
+ findExplicitReferences(
+ // Use function template in case of templated functions to visit template
+ // parameters.
+ DestTempl ? llvm::dyn_cast<Decl>(DestTempl) : llvm::dyn_cast<Decl>(Dest),
+ [&](ReferenceLoc Ref) {
+ if (Ref.Targets.size() != 1)
+ return;
+ const auto *Target =
+ llvm::cast<NamedDecl>(Ref.Targets.front()->getCanonicalDecl());
+ auto It = ParamToNewName.find(Target);
+ if (It == ParamToNewName.end())
+ return;
+ RefLocs[Target].push_back(Ref.NameLoc);
+ });
+
+ // Now try to generate edits for all the refs.
+ tooling::Replacements Replacements;
+ for (auto &Entry : RefLocs) {
+ const auto *OldDecl = Entry.first;
+ llvm::StringRef OldName = OldDecl->getName();
+ llvm::StringRef NewName = ParamToNewName[OldDecl];
+ for (SourceLocation RefLoc : Entry.second) {
+ CharSourceRange ReplaceRange;
+ // In case of unnamed parameters, we have an empty char range, whereas we
+ // have a tokenrange at RefLoc with named parameters.
+ if (OldName.empty())
+ ReplaceRange = CharSourceRange::getCharRange(RefLoc, RefLoc);
+ else
+ ReplaceRange = CharSourceRange::getTokenRange(RefLoc, RefLoc);
+ // If occurence is coming from a macro expansion, try to get back to the
+ // file range.
+ if (RefLoc.isMacroID()) {
+ ReplaceRange = Lexer::makeFileCharRange(ReplaceRange, SM, LangOpts);
+ // Bail out if we need to replace macro bodies.
+ if (ReplaceRange.isInvalid()) {
+ auto Err = llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "Cant rename parameter inside macro body.");
+ elog("define inline: {0}", Err);
+ return std::move(Err);
+ }
+ }
+
+ if (auto Err = Replacements.add(
+ tooling::Replacement(SM, ReplaceRange, NewName))) {
+ elog("define inline: Couldn't replace parameter name for {0} to {1}: "
+ "{2}",
+ OldName, NewName, Err);
+ return std::move(Err);
+ }
+ }
+ }
+ return Replacements;
+}
+
// Returns the canonical declaration for the given FunctionDecl. This will
// usually be the first declaration in current translation unit with the
// exception of template specialization.
@@ -332,6 +433,10 @@ class DefineInline : public Tweak {
"Couldn't find semicolon for target declaration.");
}
+ auto ParamReplacements = renameParameters(Target, Source);
+ if (!ParamReplacements)
+ return ParamReplacements.takeError();
+
auto QualifiedBody = qualifyAllDecls(Source);
if (!QualifiedBody)
return QualifiedBody.takeError();
@@ -354,8 +459,9 @@ class DefineInline : public Tweak {
llvm::SmallVector<std::pair<std::string, Edit>, 2> Edits;
// Edit for Target.
- auto FE = Effect::fileEdit(SM, SM.getFileID(*Semicolon),
- tooling::Replacements(SemicolonToFuncBody));
+ auto FE = Effect::fileEdit(
+ SM, SM.getFileID(*Semicolon),
+ tooling::Replacements(SemicolonToFuncBody).merge(*ParamReplacements));
if (!FE)
return FE.takeError();
Edits.push_back(std::move(*FE));
diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index f14383b24081..126aab61ada5 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -40,9 +40,9 @@
#include <vector>
using ::testing::AllOf;
+using ::testing::ElementsAre;
using ::testing::HasSubstr;
using ::testing::StartsWith;
-using ::testing::ElementsAre;
namespace clang {
namespace clangd {
@@ -1386,7 +1386,7 @@ TEST_F(DefineInlineTest, TransformFunctionTempls) {
// Template body is not parsed until instantiation time on windows, which
// results in arbitrary failures as function body becomes NULL.
ExtraArgs.push_back("-fno-delayed-template-parsing");
- for(const auto &Case : Cases)
+ for (const auto &Case : Cases)
EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
}
@@ -1429,7 +1429,7 @@ TEST_F(DefineInlineTest, TransformTypeLocs) {
}
TEST_F(DefineInlineTest, TransformDeclRefs) {
- auto Test =R"cpp(
+ auto Test = R"cpp(
namespace a {
template <typename T> class Bar {
public:
@@ -1498,6 +1498,70 @@ TEST_F(DefineInlineTest, StaticMembers) {
EXPECT_EQ(apply(Test), Expected);
}
+TEST_F(DefineInlineTest, TransformParamNames) {
+ std::pair<llvm::StringRef, llvm::StringRef> Cases[] = {
+ {R"cpp(
+ void foo(int, bool b, int T\
+est);
+ void ^foo(int f, bool x, int z) {})cpp",
+ R"cpp(
+ void foo(int f, bool x, int z){}
+ )cpp"},
+ {R"cpp(
+ #define PARAM int Z
+ void foo(PARAM);
+
+ void ^foo(int X) {})cpp",
+ "fail: Cant rename parameter inside macro body."},
+ {R"cpp(
+ #define TYPE int
+ #define PARAM TYPE Z
+ #define BODY(x) 5 * (x) + 2
+ template <int P>
+ void foo(PARAM, TYPE Q, TYPE, TYPE W = BODY(P));
+ template <int x>
+ void ^foo(int Z, int b, int c, int d) {})cpp",
+ R"cpp(
+ #define TYPE int
+ #define PARAM TYPE Z
+ #define BODY(x) 5 * (x) + 2
+ template <int x>
+ void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){}
+ )cpp"},
+ };
+ for (const auto &Case : Cases)
+ EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+ auto Test = R"cpp(
+ struct Foo {
+ struct Bar {
+ template <class, class X,
+ template<typename> class, template<typename> class Y,
+ int, int Z>
+ void foo(X, Y<X>, int W = 5 * Z + 2);
+ };
+ };
+
+ template <class T, class U,
+ template<typename> class V, template<typename> class W,
+ int X, int Y>
+ void Foo::Bar::f^oo(U, W<U>, int Q) {})cpp";
+ auto Expected = R"cpp(
+ struct Foo {
+ struct Bar {
+ template <class T, class U,
+ template<typename> class V, template<typename> class W,
+ int X, int Y>
+ void foo(U, W<U>, int Q = 5 * Y + 2){}
+ };
+ };
+
+ )cpp";
+ EXPECT_EQ(apply(Test), Expected);
+}
+
TEST_F(DefineInlineTest, TransformInlineNamespaces) {
auto Test = R"cpp(
namespace a { inline namespace b { namespace { struct Foo{}; } } }
@@ -1536,7 +1600,7 @@ TEST_F(DefineInlineTest, TokensBeforeSemicolon) {
void fo^o() { return ; })cpp",
"fail: Couldn't find semicolon for target declaration."},
};
- for(const auto& Case: Cases)
+ for (const auto &Case : Cases)
EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
}
@@ -1594,7 +1658,7 @@ TEST_F(DefineInlineTest, HandleMacros) {
void TARGET(){ return; }
)cpp"},
};
- for(const auto& Case: Cases)
+ for (const auto &Case : Cases)
EXPECT_EQ(apply(Case.first), Case.second) << Case.first;
}
More information about the cfe-commits
mailing list