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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, LGTM!



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:136
+// Removes matching instances of given token preceeding the function defition.
+llvm::Expected<tooling::Replacements>
+getDelKeywordReplacements(const FunctionDecl *FD,
----------------
bgluzman wrote:
> 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.
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`.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:137
+llvm::Expected<tooling::Replacements>
+getDelKeywordReplacements(const FunctionDecl *FD,
+                          const syntax::TokenBuffer &TokBuf,
----------------
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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:489
+    tooling::Replacements HeaderUpdates;
     const tooling::Replacement DeleteFuncBody(
         Sel.AST->getSourceManager(),
----------------
you can directly construct `Replacements` with this particular `Replacement` first, that way no need to `add` and check for `Err`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151294