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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:810-815
+  Server->applyTweak(Args.tweakID,
+                     {Args.file.file().str(),
+                      Args.selection,
+                      Args.requestedActionKinds,
+                      {},
+                      {}},
----------------
this is kind of hard to read


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:124
+  PreambleBounds getPreambleBounds() const {
+    return Preamble->Preamble.getBounds();
+  }
----------------
Unfortunately this won't work in presence of stale preambles. You can use the "patched bounds" we calculate when building a parsedast in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ParsedAST.cpp#L646; store it in `ParsedAST` in a new field, and return it here.


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:74
+    /// code action request context.
+    std::vector<std::string> RequestedActionKinds;
     // FIXME: provide a way to get sources and ASTs for other files.
----------------
prefer `llvm::ArrayRef<std:string>` here, `Selection`s are not passed in between threads. So no need to make an extra copy when creating one.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:52
+    return false;
+  if (std::find(Inputs.RequestedActionKinds.begin(),
+                Inputs.RequestedActionKinds.end(),
----------------
nit: llvm::find(Inputs.RequestedActionKinds, CodeAction::SOURCE_KIND)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:56
+    return true;
+  if (Inputs.RequestedActionKinds.empty())
+    // To accommodate clients without knowledge of source actions, we trigger
----------------
nit:
```
if (!Inputs.RequestedActionKinds.empty())
  return llvm::find(Inputs.RequestedActionKinds, CodeAction::SOURCE_KIND) != Inputs.RequestedActionKinds.end();
// To accommodate clients without knowledge of source actions, we trigger
// without checking code action kinds when inside the preamble region.
return Inputs.SelectionEnd <= Inputs.AST->getPreambleBounds().Size;
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:68
+  tooling::Replacements Replacements;
+  for (const auto *Inc : Findings.UnusedIncludes)
+    llvm::cantFail(Replacements.add(
----------------
we should respect `Config::Diagnostics::Includes::IgnoreHeader` here, similar to finding integration, similarly for missing include fixes.


================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1307
+  Server.applyTweak(
+      TweakID, {FooCpp, {}, {}, {}, {}}, [&](llvm::Expected<Tweak::Effect> E) {
+        ASSERT_TRUE(static_cast<bool>(E));
----------------
nit: Again extracting CodeActionInputs, setting filename explicitly and passing that into the call would be great here.


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/OrganizeImportsTests.cpp:35-41
+               {
+                   R"cpp(
+#include "Te^stTU.h"
+)cpp",
+                   true,
+                   {}},
+               {"void foo(^) {}", false, {}}};
----------------
nit: you can use EXPECT_AVAILABLE and EXPECT_UNAVAILABLE directly for these two cases


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h:101
+  bool isAvailable(WrappedAST &, llvm::Annotations::Range,
+                   const std::vector<std::string> & = {}) const;
   // Return code re-decorated with a single point/range.
----------------
type is not really giving any ideas what the parameter is about here, can you name it ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153769