[clangd] Remove inline Specifier for DefineOutline Tweak

[bgluzman]

Summary

inline specifiers should be removed from from the function declaration and
the newly-created implementation.

For example, take the following (working) code:

cpp
// foo.hpp
struct A {
  inline void foo() { std::cout << "hello world\n" << std::flush; }
};

// foo.cpp
#include "foo.hpp"

// main.cpp
#include "foo.hpp"

int main() {
  A a;
  a.foo();
  return 0;
}

// compile: clang++ -std=c++20 main.cpp foo.cpp -o main

After applying the tweak:

// foo.hpp
struct A {
  inline void foo();
};

// foo.cpp
#include "foo.hpp"

inline void A::foo() { std::cout << "hello world\n" << std::flush; }

// main.cpp
#include "foo.hpp"

int main() {
  A a;
  a.foo();
  return 0;
}

// compile: clang++ -std=c++20 main.cpp foo.cpp -o main

We get a link error, as expected:

/usr/bin/ld: /tmp/main-4c5d99.o: in function `main':
main.cpp:(.text+0x14): undefined reference to `A::foo()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

This revision removes these specifiers from both the header and the source file. This was identified in Github issue llvm/llvm-project#61295.

[bgluzman]

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

136

This was extracted from the DelKeyword lambda in getFunctionSourceCode since it now is used within apply as well.

Note that since this returns a tooling::Replacements, the caller must combine the result with other Replacements via tooling::Replacements::merge. This is my first time contributing to this project, so please let me know if using merge (instead of add) could cause performance issues. I can change this so that we add to a tooling::Replacements out-parameter, instead.

[kadircet]

thanks, LGTM!

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

136

thanks merge is fine, we execute this code path only when the user tries to apply the code action. hence it's performance is not as critical as prepare.

137

let's just drop the FD from signature, you can get SourceManager from syntax::TokenBuffer.

also can you update the documentation, we should rather say:

Returns replacements to delete tokens with kind `Kind` in the range `FromRange`.

nit: s/getDelKeywordReplacements/deleteTokensWithKind

486

you can directly construct Replacements with this particular Replacement first, that way no need to add and check for Err

[bgluzman]

address diff comments

[bgluzman]

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

137

Getting it from syntax::TokenBuffer is a lot nicer, thanks!

486

That's much cleaner than doing this with add, thanks!

[bgluzman]

resubmit diff update with corrected commit range

[bgluzman]

Thank you for reviewing @kadircet!

Per the MyFirstTypoFix page, I don’t have commit access. If everything looks good, could you land this patch for me and use “Brian Gluzman [e-mail]” for the commit?

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

getFunctionSourceAfterReplacements(const FunctionDecl *FD,
   // Trim the result to function definition.
   auto QualifiedFunc = tooling::applyAllReplacements(
       SM.getBufferData(SM.getMainFileID()), Replacements);
   if (!QualifiedFunc)
     return QualifiedFunc.takeError();
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
 }
 
+// Returns replacements to delete tokens with kind `Kind` in the range
+// `FromRange`. Removes matching instances of given token preceeding the
+// function defition.
+llvm::Expected<tooling::Replacements>
+deleteTokensWithKind(const syntax::TokenBuffer &TokBuf, tok::TokenKind Kind,
+                     SourceRange FromRange) {
+  tooling::Replacements DelKeywordCleanups;
+  llvm::Error Errors = llvm::Error::success();
+  bool FoundAny = false;
+  for (const auto &Tok : TokBuf.expandedTokens(FromRange)) {
+    if (Tok.kind() != Kind)
+      continue;
+    FoundAny = true;
+    auto Spelling = TokBuf.spelledForExpanded(llvm::ArrayRef(Tok));
+    if (!Spelling) {
+      Errors = llvm::joinErrors(
+          std::move(Errors),
+          error("define outline: couldn't remove `{0}` keyword.",
+                tok::getKeywordSpelling(Kind)));
+      break;
+    }
+    auto &SM = TokBuf.sourceManager();
+    CharSourceRange DelRange =
+        syntax::Token::range(SM, Spelling->front(), Spelling->back())
+            .toCharRange(SM);
+    if (auto Err =
+            DelKeywordCleanups.add(tooling::Replacement(SM, DelRange, "")))
+      Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+  }
+  if (!FoundAny) {
+    Errors = llvm::joinErrors(
+        std::move(Errors),
+        error("define outline: couldn't find `{0}` keyword to remove.",
+              tok::getKeywordSpelling(Kind)));
+  }
+
+  if (Errors)
+    return std::move(Errors);
+  return DelKeywordCleanups;
+}
+
 // Creates a modified version of function definition that can be inserted at a
 // different location, qualifies return value and function name to achieve that.
 // Contains function signature, except defaulted parameter arguments, body and
 // template parameters if applicable. No need to qualify parameters, as they are
 // looked up in the context containing the function/method.
 // FIXME: Drop attributes in function signature.
 llvm::Expected<std::string>
 getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
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>());
 
   auto DelKeyword = [&](tok::TokenKind Kind, SourceRange FromRange) {
-    bool FoundAny = false;
-    for (const auto &Tok : TokBuf.expandedTokens(FromRange)) {
-      if (Tok.kind() != Kind)
-        continue;
-      FoundAny = true;
-      auto Spelling = TokBuf.spelledForExpanded(llvm::ArrayRef(Tok));
-      if (!Spelling) {
-        Errors = llvm::joinErrors(
-            std::move(Errors),
-            error("define outline: couldn't remove `{0}` keyword.",
-                  tok::getKeywordSpelling(Kind)));
-        break;
-      }
-      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 (!FoundAny) {
-      Errors = llvm::joinErrors(
-          std::move(Errors),
-          error("define outline: couldn't find `{0}` keyword to remove.",
-                tok::getKeywordSpelling(Kind)));
+    auto DelKeywords = deleteTokensWithKind(TokBuf, Kind, FromRange);
+    if (!DelKeywords) {
+      Errors = llvm::joinErrors(std::move(Errors), DelKeywords.takeError());
+      return;
     }
+    DeclarationCleanups = DeclarationCleanups.merge(*DelKeywords);
   };
 
+  if (FD->isInlineSpecified())
+    DelKeyword(tok::kw_inline, {FD->getBeginLoc(), FD->getLocation()});
   if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
     if (MD->isVirtualAsWritten())
       DelKeyword(tok::kw_virtual, {FD->getBeginLoc(), FD->getLocation()});
     if (MD->isStatic())
       DelKeyword(tok::kw_static, {FD->getBeginLoc(), FD->getLocation()});
   }
   if (const auto *CD = dyn_cast<CXXConstructorDecl>(FD)) {
     if (CD->isExplicit())
Expected<Effect> apply(const Selection &Sel) override {
     SourceManagerForFile SMFF(*CCFile, Contents);
     const tooling::Replacement InsertFunctionDef(
         *CCFile, InsertionPoint->Offset, 0, *FuncDef);
     auto Effect = Effect::mainFileEdit(
         SMFF.get(), tooling::Replacements(InsertFunctionDef));
     if (!Effect)
       return Effect.takeError();
 
-    // FIXME: We should also get rid of inline qualifier.
-    const tooling::Replacement DeleteFuncBody(
+    tooling::Replacements HeaderUpdates(tooling::Replacement(
         Sel.AST->getSourceManager(),
         CharSourceRange::getTokenRange(*toHalfOpenFileRange(
             SM, Sel.AST->getLangOpts(),
             getDeletionRange(Source, Sel.AST->getTokens()))),
-        ";");
-    auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(),
-                                     tooling::Replacements(DeleteFuncBody));
+        ";"));
+
+    if (Source->isInlineSpecified()) {
+      auto DelInline =
+          deleteTokensWithKind(Sel.AST->getTokens(), tok::kw_inline,
+                               {Source->getBeginLoc(), Source->getLocation()});
+      if (!DelInline)
+        return DelInline.takeError();
+      HeaderUpdates = HeaderUpdates.merge(*DelInline);
+    }
+
+    auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
     if (!HeaderFE)
       return HeaderFE.takeError();
 
     Effect->ApplyEdits.try_emplace(HeaderFE->first,
                                    std::move(HeaderFE->second));
     return std::move(*Effect);
   }
 

clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp

struct {
     llvm::StringRef ExpectedSource;
   } Cases[] = {
       // Simple check
       {
           "void fo^o() { return; }",
           "void foo() ;",
           "void foo() { return; }",
       },
+      // Inline specifier.
+      {
+          "inline void fo^o() { return; }",
+          " void foo() ;",
+          " void foo() { return; }",
+      },
       // Default args.
       {
           "void fo^o(int x, int y = 5, int = 2, int (*foo)(int) = nullptr) {}",
           "void foo(int x, int y = 5, int = 2, int (*foo)(int) = nullptr) ;",
           "void foo(int x, int y , int , int (*foo)(int) ) {}",
       },
       {
           "struct Bar{Bar();}; void fo^o(Bar x = {}) {}",
llvm::StringRef ExpectedSource;
               explicit explicit Fo^o(int) {}
             };)cpp",
           R"cpp(
             struct Foo {
               explicit explicit Foo(int) ;
             };)cpp",
           "  Foo::Foo(int) {}\n",
       },
+      {
+          R"cpp(
+            struct A {
+              inline void f^oo(int) {}
+            };)cpp",
+          R"cpp(
+            struct A {
+               void foo(int) ;
+            };)cpp",
+          " void A::foo(int) {}\n",
+      },
       // Destrctors
       {
           "class A { ~A^(){} };",
           "class A { ~A(); };",
           "A::~A(){} ",
       },
   };
   for (const auto &Case : Cases) {