From 087528a331786228221d7a56a51ab97a3fcac8f1 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Tue, 1 Oct 2019 10:39:14 +0200 Subject: [PATCH] [clangd] Add "inline" keyword to prevent ODR-violations in DefineInline Reviewers: ilya-biryukov, hokein Subscribers: MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D68261 --- .../clangd/refactor/tweaks/DefineInline.cpp | 33 ++++++++++-- clang-tools-extra/clangd/unittests/TweakTests.cpp | 60 ++++++++++++++++++++++ 2 files changed, 90 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp index 3bc5df0..57690ee 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp @@ -32,6 +32,7 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/TokenKinds.h" +#include "clang/Driver/Types.h" #include "clang/Index/IndexDataConsumer.h" #include "clang/Index/IndexSymbol.h" #include "clang/Index/IndexingAction.h" @@ -360,6 +361,25 @@ const SourceLocation getBeginLoc(const FunctionDecl *FD) { return FD->getBeginLoc(); } +llvm::Optional +addInlineIfInHeader(const FunctionDecl *FD) { + // This includes inline functions and constexpr functions. + if (FD->isInlined() || llvm::isa(FD)) + return llvm::None; + // Primary template doesn't need inline. + if (FD->isTemplated() && !FD->isFunctionTemplateSpecialization()) + return llvm::None; + + const SourceManager &SM = FD->getASTContext().getSourceManager(); + llvm::StringRef FileName = SM.getFilename(FD->getLocation()); + + // If it is not a header we don't need to mark function as "inline". + if (!isHeaderFile(FileName, FD->getASTContext().getLangOpts())) + return llvm::None; + + return tooling::Replacement(SM, FD->getInnerLocStart(), 0, "inline "); +} + /// Moves definition of a function/method to its declaration location. /// Before: /// a.h: @@ -436,6 +456,7 @@ public: "Couldn't find semicolon for target declaration."); } + auto AddInlineIfNecessary = addInlineIfInHeader(Target); auto ParamReplacements = renameParameters(Target, Source); if (!ParamReplacements) return ParamReplacements.takeError(); @@ -446,6 +467,13 @@ public: const tooling::Replacement SemicolonToFuncBody(SM, *Semicolon, 1, *QualifiedBody); + tooling::Replacements TargetFileReplacements(SemicolonToFuncBody); + TargetFileReplacements = TargetFileReplacements.merge(*ParamReplacements); + if (AddInlineIfNecessary) { + if (auto Err = TargetFileReplacements.add(*AddInlineIfNecessary)) + return std::move(Err); + } + auto DefRange = toHalfOpenFileRange( SM, AST.getLangOpts(), SM.getExpansionRange(CharSourceRange::getCharRange(getBeginLoc(Source), @@ -462,9 +490,8 @@ public: llvm::SmallVector, 2> Edits; // Edit for Target. - auto FE = Effect::fileEdit( - SM, SM.getFileID(*Semicolon), - tooling::Replacements(SemicolonToFuncBody).merge(*ParamReplacements)); + auto FE = Effect::fileEdit(SM, SM.getFileID(*Semicolon), + std::move(TargetFileReplacements)); 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 d50d27a..ebeea82 100644 --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -1864,6 +1864,66 @@ TEST_F(DefineInlineTest, QualifyWithUsingDirectives) { EXPECT_EQ(apply(Test), Expected) << Test; } +TEST_F(DefineInlineTest, AddInline) { + llvm::StringMap EditedFiles; + ExtraFiles["a.h"] = "void foo();"; + apply(R"cpp(#include "a.h" + void fo^o() {})cpp", &EditedFiles); + EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents( + testPath("a.h"), "inline void foo(){}"))); + + // Check we put inline before cv-qualifiers. + ExtraFiles["a.h"] = "const int foo();"; + apply(R"cpp(#include "a.h" + const int fo^o() {})cpp", &EditedFiles); + EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents( + testPath("a.h"), "inline const int foo(){}"))); + + // No double inline. + ExtraFiles["a.h"] = "inline void foo();"; + apply(R"cpp(#include "a.h" + inline void fo^o() {})cpp", &EditedFiles); + EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents( + testPath("a.h"), "inline void foo(){}"))); + + // Constexprs don't need "inline". + ExtraFiles["a.h"] = "constexpr void foo();"; + apply(R"cpp(#include "a.h" + constexpr void fo^o() {})cpp", &EditedFiles); + EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents( + testPath("a.h"), "constexpr void foo(){}"))); + + // Class members don't need "inline". + ExtraFiles["a.h"] = "struct Foo { void foo(); }"; + apply(R"cpp(#include "a.h" + void Foo::fo^o() {})cpp", &EditedFiles); + EXPECT_THAT(EditedFiles, + testing::ElementsAre(FileWithContents( + testPath("a.h"), "struct Foo { void foo(){} }"))); + + // Function template doesn't need to be "inline"d. + ExtraFiles["a.h"] = "template void foo();"; + apply(R"cpp(#include "a.h" + template + void fo^o() {})cpp", &EditedFiles); + EXPECT_THAT(EditedFiles, + testing::ElementsAre(FileWithContents( + testPath("a.h"), "template void foo(){}"))); + + // Specializations needs to be marked "inline". + ExtraFiles["a.h"] = R"cpp( + template void foo(); + template <> void foo();)cpp"; + apply(R"cpp(#include "a.h" + template <> + void fo^o() {})cpp", &EditedFiles); + EXPECT_THAT(EditedFiles, + testing::ElementsAre(FileWithContents(testPath("a.h"), + R"cpp( + template void foo(); + template <> inline void foo(){})cpp"))); +} + TWEAK_TEST(DefineOutline); TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) { FileName = "Test.cpp"; -- 2.7.4