[PATCH] D153769: [clangd] Implement the 'Organize Imports' source action. Fix include-cleaner findings in batch.

[Viktoriia Bakalova via Phabricator via cfe-commits]

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