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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet 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);
----------------
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?


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1072-1074
+      AddFix(RemoveAllUnused);
+      AddFix(AddAllMissing);
+      AddFix(FixAll);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142967