[clang-tools-extra] 087528a - [clangd] Add "inline" keyword to prevent ODR-violations in DefineInline

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2019-12-13T10:07:18+01:00
New Revision: 087528a331786228221d7a56a51ab97a3fcac8f1

URL: https://github.com/llvm/llvm-project/commit/087528a331786228221d7a56a51ab97a3fcac8f1
DIFF: https://github.com/llvm/llvm-project/commit/087528a331786228221d7a56a51ab97a3fcac8f1.diff

LOG: [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

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 3bc5df0edbfd..57690ee3d684 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<tooling::Replacement>
+addInlineIfInHeader(const FunctionDecl *FD) {
+  // This includes inline functions and constexpr functions.
+  if (FD->isInlined() || llvm::isa<CXXMethodDecl>(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 @@ class DefineInline : public Tweak {
           "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 @@ class DefineInline : public Tweak {
 
     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 @@ 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).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 d50d27afb1c8..ebeea82864cb 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<std::string> 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 <typename T> void foo();";
+  apply(R"cpp(#include "a.h"
+              template <typename T>
+              void fo^o() {})cpp", &EditedFiles);
+  EXPECT_THAT(EditedFiles,
+              testing::ElementsAre(FileWithContents(
+                  testPath("a.h"), "template <typename T> void foo(){}")));
+
+  // Specializations needs to be marked "inline".
+  ExtraFiles["a.h"] = R"cpp(
+                            template <typename T> void foo();
+                            template <> void foo<int>();)cpp";
+  apply(R"cpp(#include "a.h"
+              template <>
+              void fo^o<int>() {})cpp", &EditedFiles);
+  EXPECT_THAT(EditedFiles,
+              testing::ElementsAre(FileWithContents(testPath("a.h"),
+                                                    R"cpp(
+                            template <typename T> void foo();
+                            template <> inline void foo<int>(){})cpp")));
+}
+
 TWEAK_TEST(DefineOutline);
 TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
   FileName = "Test.cpp";