diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp --- 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" @@ -29,6 +30,7 @@ #include "clang/Lex/Lexer.h" #include "clang/Tooling/Core/Replacement.h" #include "clang/Tooling/Syntax/Tokens.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" @@ -156,7 +158,7 @@ "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. @@ -181,7 +183,7 @@ 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)); }); @@ -206,14 +208,74 @@ 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 = CharSourceRange::getTokenRange( + AttrTokens->front().location(), AttrTokens->back().location()); + if (auto Err = + DeclarationCleanups.add(tooling::Replacement(SM, DelRange, ""))) + Errors = llvm::joinErrors(std::move(Errors), std::move(Err)); + }; + + DelAttr(FD->getAttr()); + DelAttr(FD->getAttr()); + + if (FD->isVirtualAsWritten()) { + SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()}; + bool Any = false; + + // Clang allows duplicating virtual specifiers so check for multiple + // occurances. + for (const auto &Tok : TokBuf.expandedTokens(SpecRange)) { + if (Tok.kind() != tok::kw_virtual) + continue; + Any = true; + auto Spelling = TokBuf.spelledForExpanded(llvm::makeArrayRef(Tok)); + if (!Spelling) { + 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.")); + break; + } + assert(Spelling->size() == 1); + if (auto Err = DeclarationCleanups.add(tooling::Replacement( + SM, Spelling->front().range(SM).toCharRange(SM), ""))) + Errors = llvm::joinErrors(std::move(Errors), std::move(Err)); + } + if (!Any) { + 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 --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -2068,6 +2068,80 @@ };)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); @@ -2081,6 +2155,8 @@ llvm::StringMap EditedFiles; ExtraFiles["Test.cpp"] = ""; FileName = "Test.hpp"; + ExtraArgs.push_back("-DVIRTUAL=virtual"); + ExtraArgs.push_back("-DOVER=override"); struct { llvm::StringRef Test; @@ -2118,6 +2194,42 @@ #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", + }, }; for (const auto &Case : Cases) { SCOPED_TRACE(Case.Test); @@ -2229,6 +2341,49 @@ << Case.TestHeader; } } + +TEST_F(DefineOutlineTest, FailsMacroSpecifier) { + FileName = "Test.hpp"; + ExtraFiles["Test.cpp"] = ""; + ExtraArgs.push_back("-DFINALOVER=final override"); + + std::pair 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