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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
