[llvm-branch-commits] [clang-tools-extra] bca373f - [clangd] DefineOutline won't copy virtual specifiers on methods
Hans Wennborg via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Wed Mar 4 00:15:34 PST 2020
Author: Nathan James
Date: 2020-03-04T09:11:44+01:00
New Revision: bca373f73fc82728a8335e7d6cd164e8747139ec
URL: https://github.com/llvm/llvm-project/commit/bca373f73fc82728a8335e7d6cd164e8747139ec
DIFF: https://github.com/llvm/llvm-project/commit/bca373f73fc82728a8335e7d6cd164e8747139ec.diff
LOG: [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 b2666ccca0277371a09e43a0a5a0f78029ba81e5)
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index 6d27149ef6d4..ca3d74b8dcac 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -16,6 +16,7 @@
#include "SourceCode.h"
#include "refactor/Tweak.h"
#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Attr.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
@@ -155,7 +156,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
"define outline: couldn't find a context for target");
llvm::Error Errors = llvm::Error::success();
- tooling::Replacements QualifierInsertions;
+ tooling::Replacements DeclarationCleanups;
// Finds the first unqualified name in function return type and name, then
// qualifies those to be valid in TargetContext.
@@ -180,7 +181,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
const NamedDecl *ND = Ref.Targets.front();
const std::string Qualifier = getQualification(
AST, *TargetContext, SM.getLocForStartOfFile(SM.getMainFileID()), ND);
- if (auto Err = QualifierInsertions.add(
+ if (auto Err = DeclarationCleanups.add(
tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
});
@@ -205,14 +206,72 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
assert(Tok != Tokens.rend());
DelRange.setBegin(Tok->location());
if (auto Err =
- QualifierInsertions.add(tooling::Replacement(SM, DelRange, "")))
+ DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
}
}
+ auto DelAttr = [&](const Attr *A) {
+ if (!A)
+ return;
+ auto AttrTokens =
+ TokBuf.spelledForExpanded(TokBuf.expandedTokens(A->getRange()));
+ assert(A->getLocation().isValid());
+ if (!AttrTokens || AttrTokens->empty()) {
+ Errors = llvm::joinErrors(
+ std::move(Errors),
+ llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ llvm::StringRef("define outline: Can't move out of line as "
+ "function has a macro `") +
+ A->getSpelling() + "` specifier."));
+ return;
+ }
+ CharSourceRange DelRange =
+ syntax::Token::range(SM, AttrTokens->front(), AttrTokens->back())
+ .toCharRange(SM);
+ if (auto Err =
+ DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
+ Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+ };
+
+ DelAttr(FD->getAttr<OverrideAttr>());
+ DelAttr(FD->getAttr<FinalAttr>());
+
+ if (FD->isVirtualAsWritten()) {
+ SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()};
+ bool HasErrors = true;
+
+ // Clang allows duplicating virtual specifiers so check for multiple
+ // occurances.
+ for (const auto &Tok : TokBuf.expandedTokens(SpecRange)) {
+ if (Tok.kind() != tok::kw_virtual)
+ continue;
+ auto Spelling = TokBuf.spelledForExpanded(llvm::makeArrayRef(Tok));
+ if (!Spelling) {
+ HasErrors = true;
+ break;
+ }
+ HasErrors = false;
+ CharSourceRange DelRange =
+ syntax::Token::range(SM, Spelling->front(), Spelling->back())
+ .toCharRange(SM);
+ if (auto Err =
+ DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
+ Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+ }
+ if (HasErrors) {
+ Errors = llvm::joinErrors(
+ std::move(Errors),
+ llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "define outline: Can't move out of line as "
+ "function has a macro `virtual` specifier."));
+ }
+ }
+
if (Errors)
return std::move(Errors);
- return getFunctionSourceAfterReplacements(FD, QualifierInsertions);
+ return getFunctionSourceAfterReplacements(FD, DeclarationCleanups);
}
struct InsertionPoint {
diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index 38f958c96173..adbdbe71761d 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2065,6 +2065,80 @@ TEST_F(DefineOutlineTest, ApplyTest) {
};)cpp",
"Foo::Foo(int z) __attribute__((weak)) : bar(2){}\n",
},
+ // Virt specifiers.
+ {
+ R"cpp(
+ struct A {
+ virtual void f^oo() {}
+ };)cpp",
+ R"cpp(
+ struct A {
+ virtual void foo() ;
+ };)cpp",
+ " void A::foo() {}\n",
+ },
+ {
+ R"cpp(
+ struct A {
+ virtual virtual void virtual f^oo() {}
+ };)cpp",
+ R"cpp(
+ struct A {
+ virtual virtual void virtual foo() ;
+ };)cpp",
+ " void A::foo() {}\n",
+ },
+ {
+ R"cpp(
+ struct A {
+ virtual void foo() = 0;
+ };
+ struct B : A {
+ void fo^o() override {}
+ };)cpp",
+ R"cpp(
+ struct A {
+ virtual void foo() = 0;
+ };
+ struct B : A {
+ void foo() override ;
+ };)cpp",
+ "void B::foo() {}\n",
+ },
+ {
+ R"cpp(
+ struct A {
+ virtual void foo() = 0;
+ };
+ struct B : A {
+ void fo^o() final {}
+ };)cpp",
+ R"cpp(
+ struct A {
+ virtual void foo() = 0;
+ };
+ struct B : A {
+ void foo() final ;
+ };)cpp",
+ "void B::foo() {}\n",
+ },
+ {
+ R"cpp(
+ struct A {
+ virtual void foo() = 0;
+ };
+ struct B : A {
+ void fo^o() final override {}
+ };)cpp",
+ R"cpp(
+ struct A {
+ virtual void foo() = 0;
+ };
+ struct B : A {
+ void foo() final override ;
+ };)cpp",
+ "void B::foo() {}\n",
+ },
};
for (const auto &Case : Cases) {
SCOPED_TRACE(Case.Test);
@@ -2078,6 +2152,8 @@ TEST_F(DefineOutlineTest, HandleMacros) {
llvm::StringMap<std::string> EditedFiles;
ExtraFiles["Test.cpp"] = "";
FileName = "Test.hpp";
+ ExtraArgs.push_back("-DVIRTUAL=virtual");
+ ExtraArgs.push_back("-DOVER=override");
struct {
llvm::StringRef Test;
@@ -2115,6 +2191,48 @@ TEST_F(DefineOutlineTest, HandleMacros) {
#define TARGET foo
void TARGET();)cpp",
"void TARGET(){ return; }"},
+ {R"cpp(#define VIRT virtual
+ struct A {
+ VIRT void f^oo() {}
+ };)cpp",
+ R"cpp(#define VIRT virtual
+ struct A {
+ VIRT void foo() ;
+ };)cpp",
+ " void A::foo() {}\n"},
+ {R"cpp(
+ struct A {
+ VIRTUAL void f^oo() {}
+ };)cpp",
+ R"cpp(
+ struct A {
+ VIRTUAL void foo() ;
+ };)cpp",
+ " void A::foo() {}\n"},
+ {R"cpp(
+ struct A {
+ virtual void foo() = 0;
+ };
+ struct B : A {
+ void fo^o() OVER {}
+ };)cpp",
+ R"cpp(
+ struct A {
+ virtual void foo() = 0;
+ };
+ struct B : A {
+ void foo() OVER ;
+ };)cpp",
+ "void B::foo() {}\n"},
+ {R"cpp(#define STUPID_MACRO(X) virtual
+ struct A {
+ STUPID_MACRO(sizeof sizeof int) void f^oo() {}
+ };)cpp",
+ R"cpp(#define STUPID_MACRO(X) virtual
+ struct A {
+ STUPID_MACRO(sizeof sizeof int) void foo() ;
+ };)cpp",
+ " void A::foo() {}\n"},
};
for (const auto &Case : Cases) {
SCOPED_TRACE(Case.Test);
@@ -2226,6 +2344,49 @@ TEST_F(DefineOutlineTest, QualifyFunctionName) {
<< Case.TestHeader;
}
}
+
+TEST_F(DefineOutlineTest, FailsMacroSpecifier) {
+ FileName = "Test.hpp";
+ ExtraFiles["Test.cpp"] = "";
+ ExtraArgs.push_back("-DFINALOVER=final override");
+
+ std::pair<StringRef, StringRef> Cases[] = {
+ {
+ R"cpp(
+ #define VIRT virtual void
+ struct A {
+ VIRT fo^o() {}
+ };)cpp",
+ "fail: define outline: Can't move out of line as function has a "
+ "macro `virtual` specifier."},
+ {
+ R"cpp(
+ #define OVERFINAL final override
+ struct A {
+ virtual void foo() {}
+ };
+ struct B : A {
+ void fo^o() OVERFINAL {}
+ };)cpp",
+ "fail: define outline: Can't move out of line as function has a "
+ "macro `override` specifier.\ndefine outline: Can't move out of line "
+ "as function has a macro `final` specifier."},
+ {
+ R"cpp(
+ struct A {
+ virtual void foo() {}
+ };
+ struct B : A {
+ void fo^o() FINALOVER {}
+ };)cpp",
+ "fail: define outline: Can't move out of line as function has a "
+ "macro `override` specifier.\ndefine outline: Can't move out of line "
+ "as function has a macro `final` specifier."},
+ };
+ for (const auto &Case : Cases) {
+ EXPECT_EQ(apply(Case.first), Case.second);
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
More information about the llvm-branch-commits
mailing list