Skip to content

Commit bca373f

Browse files
njames93zmodem
njames93
authored and
zmodem
committed
[clangd] DefineOutline won't copy virtual specifiers on methods
Summary: The define out of line refactor tool previously would copy the `virtual`, `override` and `final` specifier into the out of line method definition. This results in malformed code as those specifiers aren't allowed outside the class definition. Reviewers: hokein, kadircet Reviewed By: kadircet Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits Tags: #clang, #clang-tools-extra Differential Revision: https://reviews.llvm.org/D75429 (cherry picked from commit b2666cc)
1 parent 3ef42c1 commit bca373f

File tree

2 files changed

+224
-4
lines changed

2 files changed

+224
-4
lines changed

‎clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp‎

Lines changed: 63 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "SourceCode.h"
1717
#include "refactor/Tweak.h"
1818
#include "clang/AST/ASTTypeTraits.h"
19+
#include "clang/AST/Attr.h"
1920
#include "clang/AST/Decl.h"
2021
#include "clang/AST/DeclBase.h"
2122
#include "clang/AST/DeclCXX.h"
@@ -155,7 +156,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
155156
"define outline: couldn't find a context for target");
156157

157158
llvm::Error Errors = llvm::Error::success();
158-
tooling::Replacements QualifierInsertions;
159+
tooling::Replacements DeclarationCleanups;
159160

160161
// Finds the first unqualified name in function return type and name, then
161162
// qualifies those to be valid in TargetContext.
@@ -180,7 +181,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
180181
const NamedDecl *ND = Ref.Targets.front();
181182
const std::string Qualifier = getQualification(
182183
AST, *TargetContext, SM.getLocForStartOfFile(SM.getMainFileID()), ND);
183-
if (auto Err = QualifierInsertions.add(
184+
if (auto Err = DeclarationCleanups.add(
184185
tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
185186
Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
186187
});
@@ -205,14 +206,72 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
205206
assert(Tok != Tokens.rend());
206207
DelRange.setBegin(Tok->location());
207208
if (auto Err =
208-
QualifierInsertions.add(tooling::Replacement(SM, DelRange, "")))
209+
DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
209210
Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
210211
}
211212
}
212213

214+
auto DelAttr = [&](const Attr *A) {
215+
if (!A)
216+
return;
217+
auto AttrTokens =
218+
TokBuf.spelledForExpanded(TokBuf.expandedTokens(A->getRange()));
219+
assert(A->getLocation().isValid());
220+
if (!AttrTokens || AttrTokens->empty()) {
221+
Errors = llvm::joinErrors(
222+
std::move(Errors),
223+
llvm::createStringError(
224+
llvm::inconvertibleErrorCode(),
225+
llvm::StringRef("define outline: Can't move out of line as "
226+
"function has a macro `") +
227+
A->getSpelling() + "` specifier."));
228+
return;
229+
}
230+
CharSourceRange DelRange =
231+
syntax::Token::range(SM, AttrTokens->front(), AttrTokens->back())
232+
.toCharRange(SM);
233+
if (auto Err =
234+
DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
235+
Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
236+
};
237+
238+
DelAttr(FD->getAttr<OverrideAttr>());
239+
DelAttr(FD->getAttr<FinalAttr>());
240+
241+
if (FD->isVirtualAsWritten()) {
242+
SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()};
243+
bool HasErrors = true;
244+
245+
// Clang allows duplicating virtual specifiers so check for multiple
246+
// occurances.
247+
for (const auto &Tok : TokBuf.expandedTokens(SpecRange)) {
248+
if (Tok.kind() != tok::kw_virtual)
249+
continue;
250+
auto Spelling = TokBuf.spelledForExpanded(llvm::makeArrayRef(Tok));
251+
if (!Spelling) {
252+
HasErrors = true;
253+
break;
254+
}
255+
HasErrors = false;
256+
CharSourceRange DelRange =
257+
syntax::Token::range(SM, Spelling->front(), Spelling->back())
258+
.toCharRange(SM);
259+
if (auto Err =
260+
DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
261+
Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
262+
}
263+
if (HasErrors) {
264+
Errors = llvm::joinErrors(
265+
std::move(Errors),
266+
llvm::createStringError(llvm::inconvertibleErrorCode(),
267+
"define outline: Can't move out of line as "
268+
"function has a macro `virtual` specifier."));
269+
}
270+
}
271+
213272
if (Errors)
214273
return std::move(Errors);
215-
return getFunctionSourceAfterReplacements(FD, QualifierInsertions);
274+
return getFunctionSourceAfterReplacements(FD, DeclarationCleanups);
216275
}
217276

218277
struct InsertionPoint {

‎clang-tools-extra/clangd/unittests/TweakTests.cpp‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2065,6 +2065,80 @@ TEST_F(DefineOutlineTest, ApplyTest) {
20652065
};)cpp",
20662066
"Foo::Foo(int z) __attribute__((weak)) : bar(2){}\n",
20672067
},
2068+
// Virt specifiers.
2069+
{
2070+
R"cpp(
2071+
struct A {
2072+
virtual void f^oo() {}
2073+
};)cpp",
2074+
R"cpp(
2075+
struct A {
2076+
virtual void foo() ;
2077+
};)cpp",
2078+
" void A::foo() {}\n",
2079+
},
2080+
{
2081+
R"cpp(
2082+
struct A {
2083+
virtual virtual void virtual f^oo() {}
2084+
};)cpp",
2085+
R"cpp(
2086+
struct A {
2087+
virtual virtual void virtual foo() ;
2088+
};)cpp",
2089+
" void A::foo() {}\n",
2090+
},
2091+
{
2092+
R"cpp(
2093+
struct A {
2094+
virtual void foo() = 0;
2095+
};
2096+
struct B : A {
2097+
void fo^o() override {}
2098+
};)cpp",
2099+
R"cpp(
2100+
struct A {
2101+
virtual void foo() = 0;
2102+
};
2103+
struct B : A {
2104+
void foo() override ;
2105+
};)cpp",
2106+
"void B::foo() {}\n",
2107+
},
2108+
{
2109+
R"cpp(
2110+
struct A {
2111+
virtual void foo() = 0;
2112+
};
2113+
struct B : A {
2114+
void fo^o() final {}
2115+
};)cpp",
2116+
R"cpp(
2117+
struct A {
2118+
virtual void foo() = 0;
2119+
};
2120+
struct B : A {
2121+
void foo() final ;
2122+
};)cpp",
2123+
"void B::foo() {}\n",
2124+
},
2125+
{
2126+
R"cpp(
2127+
struct A {
2128+
virtual void foo() = 0;
2129+
};
2130+
struct B : A {
2131+
void fo^o() final override {}
2132+
};)cpp",
2133+
R"cpp(
2134+
struct A {
2135+
virtual void foo() = 0;
2136+
};
2137+
struct B : A {
2138+
void foo() final override ;
2139+
};)cpp",
2140+
"void B::foo() {}\n",
2141+
},
20682142
};
20692143
for (const auto &Case : Cases) {
20702144
SCOPED_TRACE(Case.Test);
@@ -2078,6 +2152,8 @@ TEST_F(DefineOutlineTest, HandleMacros) {
20782152
llvm::StringMap<std::string> EditedFiles;
20792153
ExtraFiles["Test.cpp"] = "";
20802154
FileName = "Test.hpp";
2155+
ExtraArgs.push_back("-DVIRTUAL=virtual");
2156+
ExtraArgs.push_back("-DOVER=override");
20812157

20822158
struct {
20832159
llvm::StringRef Test;
@@ -2115,6 +2191,48 @@ TEST_F(DefineOutlineTest, HandleMacros) {
21152191
#define TARGET foo
21162192
void TARGET();)cpp",
21172193
"void TARGET(){ return; }"},
2194+
{R"cpp(#define VIRT virtual
2195+
struct A {
2196+
VIRT void f^oo() {}
2197+
};)cpp",
2198+
R"cpp(#define VIRT virtual
2199+
struct A {
2200+
VIRT void foo() ;
2201+
};)cpp",
2202+
" void A::foo() {}\n"},
2203+
{R"cpp(
2204+
struct A {
2205+
VIRTUAL void f^oo() {}
2206+
};)cpp",
2207+
R"cpp(
2208+
struct A {
2209+
VIRTUAL void foo() ;
2210+
};)cpp",
2211+
" void A::foo() {}\n"},
2212+
{R"cpp(
2213+
struct A {
2214+
virtual void foo() = 0;
2215+
};
2216+
struct B : A {
2217+
void fo^o() OVER {}
2218+
};)cpp",
2219+
R"cpp(
2220+
struct A {
2221+
virtual void foo() = 0;
2222+
};
2223+
struct B : A {
2224+
void foo() OVER ;
2225+
};)cpp",
2226+
"void B::foo() {}\n"},
2227+
{R"cpp(#define STUPID_MACRO(X) virtual
2228+
struct A {
2229+
STUPID_MACRO(sizeof sizeof int) void f^oo() {}
2230+
};)cpp",
2231+
R"cpp(#define STUPID_MACRO(X) virtual
2232+
struct A {
2233+
STUPID_MACRO(sizeof sizeof int) void foo() ;
2234+
};)cpp",
2235+
" void A::foo() {}\n"},
21182236
};
21192237
for (const auto &Case : Cases) {
21202238
SCOPED_TRACE(Case.Test);
@@ -2226,6 +2344,49 @@ TEST_F(DefineOutlineTest, QualifyFunctionName) {
22262344
<< Case.TestHeader;
22272345
}
22282346
}
2347+
2348+
TEST_F(DefineOutlineTest, FailsMacroSpecifier) {
2349+
FileName = "Test.hpp";
2350+
ExtraFiles["Test.cpp"] = "";
2351+
ExtraArgs.push_back("-DFINALOVER=final override");
2352+
2353+
std::pair<StringRef, StringRef> Cases[] = {
2354+
{
2355+
R"cpp(
2356+
#define VIRT virtual void
2357+
struct A {
2358+
VIRT fo^o() {}
2359+
};)cpp",
2360+
"fail: define outline: Can't move out of line as function has a "
2361+
"macro `virtual` specifier."},
2362+
{
2363+
R"cpp(
2364+
#define OVERFINAL final override
2365+
struct A {
2366+
virtual void foo() {}
2367+
};
2368+
struct B : A {
2369+
void fo^o() OVERFINAL {}
2370+
};)cpp",
2371+
"fail: define outline: Can't move out of line as function has a "
2372+
"macro `override` specifier.\ndefine outline: Can't move out of line "
2373+
"as function has a macro `final` specifier."},
2374+
{
2375+
R"cpp(
2376+
struct A {
2377+
virtual void foo() {}
2378+
};
2379+
struct B : A {
2380+
void fo^o() FINALOVER {}
2381+
};)cpp",
2382+
"fail: define outline: Can't move out of line as function has a "
2383+
"macro `override` specifier.\ndefine outline: Can't move out of line "
2384+
"as function has a macro `final` specifier."},
2385+
};
2386+
for (const auto &Case : Cases) {
2387+
EXPECT_EQ(apply(Case.first), Case.second);
2388+
}
2389+
}
22292390
} // namespace
22302391
} // namespace clangd
22312392
} // namespace clang

0 commit comments

Comments
 (0)