[PATCH] D151294: [clangd] Remove inline Specifier for DefineOutline Tweak

[Brian Gluzman via Phabricator via cfe-commits]

bgluzman created this revision.
bgluzman added reviewers: kadircet, sammccall.
Herald added a subscriber: arphaman.
Herald added a project: All.
bgluzman requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

`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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151294

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -136,6 +136,12 @@
   "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) {}",
@@ -319,6 +325,17 @@
 };)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^(){} };",
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -131,6 +131,49 @@
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
 }
 
+// Returns Replacements deleting given keywords from function defintion.
+// Removes matching instances of given token preceeding the function defition.
+llvm::Expected
+getDelKeywordReplacements(const FunctionDecl *FD,
+  const syntax::TokenBuffer &TokBuf,
+  tok::TokenKind Kind, SourceRange FromRange) {
+  auto &AST = FD->getASTContext();
+  auto &SM = AST.getSourceManager();
+
+  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;
+}
+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
@@ -251,34 +294,16 @@
   DelAttr(FD->getAttr());
 
   auto DelKeyword = [&](tok::Tok

[PATCH] D151294: [clangd] Remove inline Specifier for DefineOutline Tweak

[Brian Gluzman via Phabricator via cfe-commits]

bgluzman added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:136
+// Removes matching instances of given token preceeding the function defition.
+llvm::Expected
+getDelKeywordReplacements(const FunctionDecl *FD,

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 `Replacement`s 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151294/new/

https://reviews.llvm.org/D151294

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151294: [clangd] Remove inline Specifier for DefineOutline Tweak

[Brian Gluzman via Phabricator via cfe-commits]

bgluzman updated this revision to Diff 525715.
bgluzman added a comment.

address diff comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151294/new/

https://reviews.llvm.org/D151294

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


Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -131,15 +131,12 @@
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
 }
 
-// Returns Replacements deleting given keywords from function defintion.
-// Removes matching instances of given token preceeding the function defition.
+// Returns replacements to delete tokens with kind `Kind` in the range
+// `FromRange`. Removes matching instances of given token preceeding the
+// function defition.
 llvm::Expected
-getDelKeywordReplacements(const FunctionDecl *FD,
-  const syntax::TokenBuffer &TokBuf,
-  tok::TokenKind Kind, SourceRange FromRange) {
-  auto &AST = FD->getASTContext();
-  auto &SM = AST.getSourceManager();
-
+deleteTokensWithKind(const syntax::TokenBuffer &TokBuf, tok::TokenKind Kind,
+ SourceRange FromRange) {
   tooling::Replacements DelKeywordCleanups;
   llvm::Error Errors = llvm::Error::success();
   bool FoundAny = false;
@@ -155,6 +152,7 @@
 tok::getKeywordSpelling(Kind)));
   break;
 }
+auto &SM = TokBuf.sourceManager();
 CharSourceRange DelRange =
 syntax::Token::range(SM, Spelling->front(), Spelling->back())
 .toCharRange(SM);
@@ -294,7 +292,7 @@
   DelAttr(FD->getAttr());
 
   auto DelKeyword = [&](tok::TokenKind Kind, SourceRange FromRange) {
-auto DelKeywords = getDelKeywordReplacements(FD, TokBuf, Kind, FromRange);
+auto DelKeywords = deleteTokensWithKind(TokBuf, Kind, FromRange);
 if (!DelKeywords) {
   Errors = llvm::joinErrors(std::move(Errors), DelKeywords.takeError());
   return;
@@ -485,20 +483,17 @@
 if (!Effect)
   return Effect.takeError();
 
-tooling::Replacements HeaderUpdates;
-const tooling::Replacement DeleteFuncBody(
+tooling::Replacements HeaderUpdates(tooling::Replacement(
 Sel.AST->getSourceManager(),
 CharSourceRange::getTokenRange(*toHalfOpenFileRange(
 SM, Sel.AST->getLangOpts(),
 getDeletionRange(Source, Sel.AST->getTokens(,
-";");
-if (auto Err = HeaderUpdates.add(DeleteFuncBody))
-  return Err;
+";"));
 
 if (Source->isInlineSpecified()) {
-  auto DelInline = getDelKeywordReplacements(
-  Source, Sel.AST->getTokens(), tok::kw_inline,
-  {Source->getBeginLoc(), Source->getLocation()});
+  auto DelInline =
+  deleteTokensWithKind(Sel.AST->getTokens(), tok::kw_inline,
+   {Source->getBeginLoc(), Source->getLocation()});
   if (!DelInline)
 return DelInline.takeError();
   HeaderUpdates = HeaderUpdates.merge(*DelInline);


Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -131,15 +131,12 @@
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
 }
 
-// Returns Replacements deleting given keywords from function defintion.
-// Removes matching instances of given token preceeding the function defition.
+// Returns replacements to delete tokens with kind `Kind` in the range
+// `FromRange`. Removes matching instances of given token preceeding the
+// function defition.
 llvm::Expected
-getDelKeywordReplacements(const FunctionDecl *FD,
-  const syntax::TokenBuffer &TokBuf,
-  tok::TokenKind Kind, SourceRange FromRange) {
-  auto &AST = FD->getASTContext();
-  auto &SM = AST.getSourceManager();
-
+deleteTokensWithKind(const syntax::TokenBuffer &TokBuf, tok::TokenKind Kind,
+ SourceRange FromRange) {
   tooling::Replacements DelKeywordCleanups;
   llvm::Error Errors = llvm::Error::success();
   bool FoundAny = false;
@@ -155,6 +152,7 @@
 tok::getKeywordSpelling(Kind)));
   break;
 }
+auto &SM = TokBuf.sourceManager();
 CharSourceRange DelRange =
 syntax::Token::range(SM, Spelling->front(), Spelling->back())
 .toCharRange(SM);
@@ -294,7 +292,7 @@
   DelAttr(FD->getAttr());
 
   auto DelKeyword = [&](tok::TokenKind Kind, SourceRange FromRange) {
-auto DelKeywords = getDelKeywordReplacements(FD, TokBuf, Kind, FromRange);
+auto DelKeywords = deleteTokensWithKind(TokBuf, Kind, FromRange);
 if (!DelKeywords) {
   Errors = llvm::joinError

[PATCH] D151294: [clangd] Remove inline Specifier for DefineOutline Tweak

[Brian Gluzman via Phabricator via cfe-commits]

bgluzman added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:137
+llvm::Expected
+getDelKeywordReplacements(const FunctionDecl *FD,
+  const syntax::TokenBuffer &TokBuf,

kadircet wrote:
> 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
Getting it from `syntax::TokenBuffer` is a lot nicer, thanks!



Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:489
+tooling::Replacements HeaderUpdates;
 const tooling::Replacement DeleteFuncBody(
 Sel.AST->getSourceManager(),

kadircet wrote:
> you can directly construct `Replacements` with this particular `Replacement` 
> first, that way no need to `add` and check for `Err`
That's much cleaner than doing this with `add`, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151294/new/

https://reviews.llvm.org/D151294

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151294: [clangd] Remove inline Specifier for DefineOutline Tweak

[Brian Gluzman via Phabricator via cfe-commits]

bgluzman updated this revision to Diff 525722.
bgluzman added a comment.

resubmit diff update with corrected commit range


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151294/new/

https://reviews.llvm.org/D151294

Files:
  clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
  clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -136,6 +136,12 @@
   "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) {}",
@@ -319,6 +325,17 @@
 };)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^(){} };",
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -131,6 +131,47 @@
   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
+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
@@ -251,34 +292,16 @@
   DelAttr(FD->getAttr());
 
   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_inlin

[PATCH] D151294: [clangd] Remove inline Specifier for DefineOutline Tweak

[Brian Gluzman via Phabricator via cfe-commits]

bgluzman added a comment.

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 bgluz...@gmail.com” for the commit?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151294/new/

https://reviews.llvm.org/D151294

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits