[PATCH] D142967: [clangd] Introduce source.organizeImports code action.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1035
   }
+  if (KindAllowed(CodeAction::SOURCE_ORGANIZE_IMPORT)) {
+    std::lock_guard<std::mutex> Lock(FixItsMutex);
----------------
kadircet wrote:
> instead of doing this in here, what about introducing a new "tweak" that'll perform include-cleaner fixes?
> Pros are:
> - We can use it in places that don't use LSPServer.
> - We can later on extend "organize imports" behaviour to do other things if needed.
> - Results will be always up-to-date, as we can use the AST and compute the fixes. rather than making use of the "latest" cached version.
> - Implementation would be a little bit neater (and contained), as we can just re-use the existing components in IncludeCleaner, rather than doing some ad-hoc matching & retrieval.
> 
> WDYT?
I think it is a matter of how we view it (whether it is a fix for all diagnostics or a tweak).

but +1, yeah, this looks like a good idea to me.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1072-1074
+      AddFix(RemoveAllUnused);
+      AddFix(AddAllMissing);
+      AddFix(FixAll);
----------------
kadircet wrote:
> what does the UI look like when there are multiple code actions with "organize imports" kind?
> e.g. 1&2 can actually be applied together, but 3rd one can't be combined with others, and in theory these can also be triggered on save (if the user opts in). so I am not sure if having multiple code actions, especially when they can't be merged together, really makes much sense on the editor side.
> what does the UI look like when there are multiple code actions with "organize imports" kind?

Similar to the normal code action, if you trigger it via the right context menu (`SourceAction`), a window with 3 alternatives will pop up, so that you can select one of them (verified on VSCode).

> e.g. 1&2 can actually be applied together, but 3rd one can't be combined with others, and in theory these can also be triggered on save (if the user opts in). so I am not sure if having multiple code actions, especially when they can't be merged together, really makes much sense on the editor side.

Good point.  The VSocde `codeActionsOnSave` behavior is like what you said -- it applies each fix one by one (because we trigger preview window per fix, so 3 preview windows will be triggered in sequence).

I don't see the best way to fix it (from the `codeAction` requests are identical from the one sent via the context menu), one option is to send only a `fixAll` codeAction rather than 3.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142967