VitaNuo marked 10 inline comments as done.
VitaNuo added a comment.
Thank you for the comments!
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:752
auto Action = [File = File.str(), Sel, TweakID = TweakID.str(),
- CB = std::move(CB),
+ CB = std::move(CB), ActionKinds,
this](Expected<InputsAndAST> InpAST) mutable {
----------------
kadircet wrote:
> we're capturing `ActionKinds` as a reference, but Action is going to execute
> async. you can instead change the input to be `llvm::ArrayRef<std::string>
> ActionKinds` and capture it by value with `ActionKinds = ActionKinds.vec()`
>
> nit: even better if you just changed the signature here to look like
> `applyTweak(std::string TweakID, CodeActionInputs Inputs,
> Callback<Tweak::Effect> CB)`, as we want to consume all of these by value
> anyways. Then you can just move `TweakID` and `Inputs` into the `Action`.
Ok, I can change the signature, but it seems like I still need to move
individual members of `CodeActionInputs` into the callback separately. `File`
cannot be moved, since it's also needed after the callback as an argument to
`runWithAST`.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:56
+ return false;
+ Range PreambleRange;
+ PreambleRange.start =
----------------
kadircet wrote:
> kadircet wrote:
> > relying on `MainFileIncludes` for preamble range is not correct in general.
> > we can have includes way down inside the main file that'll be part of this
> > vector, but not preamble (also it's not sorted explicitly).
> >
> > we should instead use `ComputePreambleBounds` (nit: we can also store it in
> > ParsedAST, instead of relexing the file one more time with each CodeAction
> > request)
> Having a comment for reason would also be helpful, something like `To
> accommodate clients without knowledge of source actions, we trigger without
> checking code action kinds when inside the preamble region`.
> nit: we can also store it in ParsedAST
Seems like the data is already there, I just need to expose it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153769/new/
https://reviews.llvm.org/D153769
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
