kadircet added inline 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 {
----------------
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`.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:49
+bool OrganizeImports::prepare(const Tweak::Selection &Inputs) {
+ if (std::find(Inputs.RequestedActionKinds.begin(),
+ Inputs.RequestedActionKinds.end(),
----------------
can we also bail out for ObjC
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:56
+ return false;
+ Range PreambleRange;
+ PreambleRange.start =
----------------
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)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:56
+ return false;
+ Range PreambleRange;
+ PreambleRange.start =
----------------
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`.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:63
+ offsetToPosition(Inputs.Code, Inputs.SelectionEnd)};
+ return SelectionRange.start <= PreambleRange.end &&
+ PreambleRange.start <= SelectionRange.end;
----------------
we should also make sure that `RequestedActionKinds` is empty in this case.
Because we don't want to offer a code action of kind Source, if user is
explicitly asking for some other kind(s).
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:72
+ for (const auto *Inc : Findings.UnusedIncludes)
+ if (auto Err = Replacements.add(
+ tooling::Replacement{MainFilePath, UINT_MAX, 1, Inc->Written}))
----------------
nit: insertions of Replacements with offset `UINT_MAX` can't fail. so you just
wrap this inside `llvm::cantFail` instead.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:87
+ SM.getFileEntryForID(SM.getMainFileID())});
+ if (auto Err = Replacements.add(tooling::Replacement{
+ MainFilePath, UINT_MAX, 0, "#include " + Spelling}))
----------------
same here for never failing
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:92
+
+ if (Replacements.empty())
+ return Tweak::Effect{"No edits to apply.", {}};
----------------
i think we should perform this check on `Final` instead (as replacements can
still go empty after cleanups, and empty set of replacements will result in an
empty set anyways)
================
Comment at:
clang-tools-extra/clangd/unittests/tweaks/OrganizeImportsTests.cpp:19
+
+TEST_F(OrganizeImportsTest, All) {
+ Header = "void foo();";
----------------
can you also add some availability tests ?
================
Comment at: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp:73
+ Tweak::Selection S(Index, AST, Range.Begin, Range.End, std::move(ST),
+ FS, {std::string{CodeAction::SOURCE_KIND}});
+ if (auto T = prepareTweak(TweakID, S, nullptr)) {
----------------
ATM this is passing SOURCE_KIND for all tweaks, which is conceptually wrong.
can you instead take this in as a parameter?
for `TweakTest::apply`, we can default it to be an empty vector, and just
update `OrganizeImportsTests` to pass `{CodeAction::SOURCE_KIND}`;
for `TweakTest::isAvailable`, we can update `EXPECT_AVAILABLE_` to pass in `{}`
and call `isAvailable` directly in `OrganizeImportsTests` with `SOURCE_KIND`.
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
