kadircet added a comment.
FYI, i don't think you uploaded the new version of the patch
================
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 {
----------------
VitaNuo wrote:
> 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`.
yeah, it's fine to make a copy
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:56
+ return false;
+ Range PreambleRange;
+ PreambleRange.start =
----------------
VitaNuo wrote:
> 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.
> > nit: we can also store it in ParsedAST
> Seems like the data is already there, I just need to expose it.
not really, we calculate bounds when building a `ParsedAST` but we don't store
it anywhere.
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
