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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
