This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE352494: [clangd] Interfaces for writing code tweaks
(authored by ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D56267?vs=184039&id=184074#toc
Repository:
rCTE Clang T
ilya-biryukov updated this revision to Diff 184039.
ilya-biryukov added a comment.
- Update license headers in new files
- Add an empty cpp file to avoid cmake errors due to empty inputs
- clang-format
- Update the 'fixits-command.test' to unbreak it (the line number was larger
than the number of
ilya-biryukov added inline comments.
Comment at: clang-tools-extra/clangd/Protocol.cpp:425
+const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_TWEAK =
+"clangd.applyCodeAction";
+
sammccall wrote:
> tweak or applyTweak
Done. Thanks for spotting this
ilya-biryukov updated this revision to Diff 183949.
ilya-biryukov marked 7 inline comments as done.
ilya-biryukov added a comment.
- s/applyCodeAction/applyTweak
- Added a comment to TweakArgs
- Added a unit test for sourceLocationInMainFile
- Extract a 'validateRegistry' function
- Do not log Can
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/Protocol.cpp:425
+const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_TWEAK =
+"clangd.applyCodeAction";
+
---
ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:27
+std::vector> prepareTweaks(const Tweak::Selection &S) {
+#ifndef NDEBUG
+ {
Please note I added these assertions here
ilya-biryukov marked an inline comment as not done.
ilya-biryukov added inline comments.
Comment at: clangd/refactor/Tweak.h:40
+ struct Selection {
+static llvm::Optional create(llvm::StringRef File,
+llvm::StringRef Code,
---
ilya-biryukov updated this revision to Diff 182870.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.
- Inline Tweak::Selection::create()
- Define Tweak::id() in REGISTER_TWEAK.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.or
sammccall added a comment.
Can live with `applyTweak` etc, though it makes me sad.
Comment at: clangd/refactor/Tweak.h:40
+ struct Selection {
+static llvm::Optional create(llvm::StringRef File,
+llvm::StringRef Code,
---
ilya-biryukov updated this revision to Diff 182534.
ilya-biryukov marked 10 inline comments as done.
ilya-biryukov added a comment.
- Update comments
- Return an ID+Title instead of a Tweak object
- Rename codeAction -> tweak everywhere
- Remove Tweak::Selection::File
- Return Expected instead of
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:363
+ return CB(A.takeError());
+return CB((*A)->apply(*Inputs));
+ };
sammccall wrote:
> we should `format::cleanUpAroundReplacements`... fine to leave this as a FIXME
Added a FIX
sammccall added inline comments.
Comment at: clangd/ClangdServer.cpp:339
+ return CB(llvm::createStringError(llvm::inconvertibleErrorCode(),
+"could not create action context"));
+CB(prepareTweaks(*Inputs));
(actio
sammccall added a comment.
This looks really good, everything else is pretty superficial I think.
I think we'll want to add one lit test for the whole two-step tweak workflow,
as well as TestTU/annotation-based unit-tests for each tweak.
As this patch has no actual tweaks in it, we can work out
ilya-biryukov added a comment.
This should be ready for another round of review. Let me know if I missed any
of the older comments.
There are two more things that I'd like to do before this lands:
- go through the docs again and update/simplify them to make sure they're in
sync with the new int
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:346
+ {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND,
+ ExecuteCommandParams::CLANGD_APPLY_CODE_ACTION}},
}},
sammccall wrote:
> Seems a
ilya-biryukov updated this revision to Diff 182315.
ilya-biryukov marked 9 inline comments as done.
ilya-biryukov added a comment.
- Remove intermediate StringMap, use registry directly
- Rename codeAction to codeTweak in ClangdServer
- Rename a helper to get position to sourceLocationInMainFile
-
ilya-biryukov added a comment.
Haven't yet addressed all the comments, but switched to use the "object
library" (i.e. a collection of .o files) to make sure linker does not optimize
away global ctors required by registry.
Comment at: clangd/refactor/Tweak.cpp:38
+namespace {
ilya-biryukov updated this revision to Diff 182272.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Build tweaks as an object library. To make sure linker does not optimize away
global constructors.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
sammccall added a comment.
(Mostly just comments on Tweak, I think the comments on other parts still apply)
Comment at: clangd/SourceCode.h:58
+/// Return the file location, corresponding to \p P. Note that one should take
+/// care to avoid comparing the result with expansio
ilya-biryukov updated this revision to Diff 182078.
ilya-biryukov added a comment.
Herald added a subscriber: mgrang.
- Rename ActionProvider to Tweak
- Update an interface of Tweak
- ActionInputs -> Tweak::Selection
- Remove CodeAction.h, use tweak registry instead
Repository:
rCTE Clang Tool
sammccall added a comment.
Offline discussion:
- CRTP doesn't buy anything. Simple inheritance is less clean than current
function-passing version but probably more familiar.
- `Tweak` is a good name: short, evocative but not commonly used, works as a
noun and a verb.
- Registry might be a good
sammccall added a comment.
Name ideas:
- "Refactoring" - I think the main drawback is the conflicts with potential
global-refactorings that we do in clangd. I don't think the conflict with
`tooling/refactor` is bad enough to avoid this. I also don't think "some of
these don't look like refacto
sammccall added a comment.
To be a bit more concrete...
One approach which won't win any OO design points but might be quite easy to
navigate: combine the ActionProvider and PreparedAction into one class.
class SwapIfBranches : public MiniRefactoringThing {
SwapIfBranches(); // must have
sammccall added a comment.
High level comments:
- this is an important concept and in hindsight it's amazing we didn't have it
yet!
- I don't get a clear sense of the boundaries between "code action",
"refactoring", and "action". There are also "prepared actions" and "instant
actions". I thi
ilya-biryukov added a comment.
I've added a few sample actions, please take a look.
The major thing that's missing from the design is how we can define an
interface for actions to get the nodes they interested in from the AST without
doing an AST traversal in each of the actions separately. I h
ilya-biryukov updated this revision to Diff 181321.
ilya-biryukov added a comment.
- Remove 'using namespace llvm'
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clangd/CMakeLists.txt
clangd/Clan
ilya-biryukov updated this revision to Diff 181309.
ilya-biryukov added a comment.
- Put more AST-centric information into ActionInputs
- Restructure and refactor the code
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm
ilya-biryukov added a comment.
That's the result of me playing around with writing a small code action for
clangd.
It's not final yet, e.g. missing tests and example actions, but still wanted to
drop the change here to make sure it's not lost, I believe we'll need something
like this in the nea
ilya-biryukov created this revision.
ilya-biryukov added reviewers: kadircet, ioeric, hokein, sammccall.
Herald added subscribers: arphaman, jkorous, MaskRay, mgorny.
The code actions can be run in two stages:
- Stage 1. Decides whether the action is available to the user and collects all
the in
29 matches
Mail list logo
