[clangd] Add "inline" keyword to prevent ODR-violations in DefineInline

[kadircet]

• Rebase

[merge_guards_bot]

Build result: pass - 60290 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

[kadircet]

• Rebase

[merge_guards_bot]

Build result: pass - 60656 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[hokein]

I think we probably want to do the vice-versa thing (remove the inline keyword) in define-outline.

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

310

nit: use isHeaderFile in SourceCode.h.

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

1354

could you add a test case for partial template specializations? I think we don't need the inline for that.

[kadircet]

• Address comments and rebase

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

1354

there's no such thing as partial specialization for functions, and we don't allow inlining inside templated classes.

[merge_guards_bot]

Unit tests: pass. 60834 tests passed, 0 failed and 726 were skipped.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or apply this patch.

Build artifacts: console-log.txt, CMakeCache.txt, clang-format.patch, test-results.xml, diff.json

clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp

 #include "clang/AST/Stmt.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/LangOptions.h"
 #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"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/Token.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Sema.h"

 // template keyword for templated functions.
 const SourceLocation getBeginLoc(const FunctionDecl *FD) {
   // Include template parameter list.
   if (auto *FTD = FD->getDescribedFunctionTemplate())
     return FTD->getBeginLoc();
   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());
+
+  auto Type = driver::types::lookupTypeForExtension(
+      llvm::sys::path::extension(FileName).substr(1));
+
+  // If it is not a header we don't need to mark function as "inline".
+  if (Type == driver::types::TY_INVALID ||
+      !driver::types::onlyPrecompileType(Type)) {
+    return llvm::None;
+  }
+
+  return tooling::Replacement(SM, FD->getInnerLocStart(), 0, "inline ");
+}
+
 /// Moves definition of a function/method to its declaration location.
 /// Before:
 /// a.h:
 ///   void foo();
 ///
 /// a.cc:
 ///   void foo() { return; }
 ///
Expected<Effect> apply(const Selection &Sel) override {
 
     auto SemiColon = getSemiColonForDecl(Target);
     if (!SemiColon) {
       return llvm::createStringError(
           llvm::inconvertibleErrorCode(),
           "Couldn't find semicolon for target declaration");
     }
 
+    auto AddInlineIfNecessary = addInlineIfInHeader(Target);
     auto ParamReplacements = renameParameters(Target, Source);
     if (!ParamReplacements)
       return ParamReplacements.takeError();
 
     auto QualifiedBody = qualifyAllDecls(Source);
     if (!QualifiedBody)
       return QualifiedBody.takeError();
 
     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);
+    }
+
     SourceLocation BeginLoc = getBeginLoc(Source);
     unsigned int SourceLen =
         SM.getFileOffset(Source->getEndLoc()) - SM.getFileOffset(BeginLoc) + 1;
     const tooling::Replacement DeleteFuncBody(SM, BeginLoc, SourceLen, "");
 
     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));
 
     // Edit for Source.
     if (!SM.isWrittenInSameFile(Source->getBeginLoc(), Target->getBeginLoc())) {
       // Generate a new edit if the Source and Target are in different files.
       auto FE = Effect::fileEdit(SM, SM.getFileID(Sel.Cursor),

clang-tools-extra/clangd/unittests/TweakTesting.cpp

std::string TweakTest::apply(llvm::StringRef MarkedCode) {
   for (auto &It : Result->ApplyEdits) {
     auto NewText = It.second.apply();
     if (!NewText)
       return "bad edits: " + llvm::toString(NewText.takeError());
     llvm::StringRef Unwrapped = unwrap(Context, *NewText);
     if (It.first() == testPath(TU.Filename))
       EditedMainFile = Unwrapped;
     else
-      EditedFiles.try_emplace(It.first(), Unwrapped.str());
+      EditedFiles[It.first()] = Unwrapped.str();
   }
   return EditedMainFile;
 }
 
 ::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header,
                           ExtraFiles);
 }

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

struct Foo {
                   template<typename> class V, template<typename> class W,
                   int X, int Y>
         void foo(){}
       };
     };
 
     )cpp");
 }
+
+TEST_F(DefineInlineTest, AddInline) {
+  ExtraFiles["a.h"] = "void foo();";
+  apply(R"cpp(#include "a.h"
+              void fo^o() {})cpp");
+  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");
+  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");
+  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");
+  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");
+  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");
+  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");
+  EXPECT_THAT(EditedFiles,
+              testing::ElementsAre(FileWithContents(testPath("a.h"),
+                                                    R"cpp(
+                            template <typename T> void foo();
+                            template <> inline void foo<int>(){})cpp")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang