kadircet added a comment.
there are unittests for tweaks under
`clang-tools-extra/clangd/unittests/tweaks/` can you create one for
`OrganizeImports` there and test this in there?
i've got another concern around triggering of this interaction.
in theory code-action context has a `only` field, dictating which code actions
should be surfaced, but when it's unset (as it's usually not set by clients and
hard to understand semantics of code action kinds) clangd just provides all of
the available code actions for a given selection.
since organize imports (and other source actions) are going to be available for
all selections, it means we'll now have some code action response all the time
and clients that don't know about semantics for "source" code action kind, will
likely just show it in their UIs. so we need to be somewhat careful about
triggering.
two alternatives that I can think of are:
1- show "source" kind code actions, only when they're explicitly requested.
2- make use of trigger kind and return these only when code actions are
"explicitly" triggered.
1 has the downside of never showing up on editors that don't know about
"source" code action kind, and also complicates behaviour on clangd side.
2 has the downside of being introduced very recently (trigger kind is made part
of the spec at LSP 3.17), hence it'll likely be absent in old clients.
so my inclination is towards alternative 2), which at least limits the blast
radius and gives clients a reasonably easy way to control the behavior. WDYT?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:24-25
+
+namespace clang {
+namespace clangd {
+namespace {
----------------
nit: `namespace clang::clangd {`
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:45-49
+std::string OrganizeImports::title() const {
+ return "Remove unused and add missing includes";
+}
+
+bool OrganizeImports::prepare(const Selection &Inputs) { return true; }
----------------
nit: i'd inline these into the class definition as well
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:51
+
+Expected<OrganizeImports::Effect>
+OrganizeImports::apply(const Selection &Inputs) {
----------------
s/OrganizeImports::Effect/Tweak::Effect
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:62
+ const auto &SM = Inputs.AST->getSourceManager();
+ std::set<std::string> Spellings;
+ for (const auto &Missing : Findings.MissingIncludes)
----------------
prefer `llvm::StringSet<>`
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:64-67
+ for (const auto &P : Missing.Providers)
+ Spellings.insert(include_cleaner::spellHeader(
+ {P, Inputs.AST->getPreprocessor().getHeaderSearchInfo(),
+ SM.getFileEntryForID(SM.getMainFileID())}));
----------------
this will insert all providers for a symbol (e.g. if we have both `foo.h` and
`bar.h`, we'll insert both).
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:75
+ auto FileStyle =
+ format::getStyle(format::DefaultFormatStyle, Inputs.AST->tuPath(),
+ format::DefaultFallbackStyle, Inputs.Code, Inputs.FS);
----------------
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:77
+ format::DefaultFallbackStyle, Inputs.Code, Inputs.FS);
+ if (!FileStyle)
+ FileStyle = format::getLLVMStyle();
----------------
unfortunately we actually have to consume the error here, as `getStyle` returns
an `Expected<T>` and destroying an unchecked `Expected` is undefined behavior :/
something like `elog("Couldn't get style for {0}: {1}", MainFilePath,
FileStyle.takeError());` should do the trick.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:79-83
+ if (auto Final = format::cleanupAroundReplacements(Inputs.Code, Replacements,
+ *FileStyle))
+ return Effect::mainFileEdit(SM, *Final);
+ else
+ return Final.takeError();
----------------
nit:
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
