sammccall added a comment.
Herald added a subscriber: cfe-commits.
Sorry about taking forever to get around to this, I kept looking at it and not
understanding it.
IIUC the idea is:
- tweaks depend on various data from the codeAction request
- but we only have the codeAction request when enumerating, not when executing
- so we jam all the data needed to recalculate the tweak into the command args
- now tweaks depend on codeactioncontext.diagnostics, so jam that in command
args too
**This definitely works and we can move forward with it** but it raises some
ideas I'd like to talk through...
---
Extending this pattern this way feels wasteful: while file/range/tweakID is
always needed to recompute the tweak, diagnostics almost never are. This
inefficiency probably **isn't** really important (though amusingly if you have
N diagnostics overlapping a point where M code actions are available, the
`codeAction` response will contain N*M diagnostics...)
And the advantage is, the logic to extract info out of diagnostics gets exactly
reused between enumerate & apply, which is the Tweak design.
But it's worth **considering** the alternative. Fundamentally this is that
enumerate extracts the info from diagnostics and passes it directly to apply,
which is the Code Action design. In this world the tweak only sees the
CodeActionContext when enumerating and can output some JSON data, and when
applying it sees the JSON data instead.
The reason this is worth entertaining is the Code Action design supports
multiple instances of the same tweak, where the Tweak design does not. To
support three different "override abstract method X" actions, each needs to
pass its own value of X as data from enumerate -> apply so that they end up
doing different things. And if we had this mechanism, I think would feel like
the natural way to handle diagnostics.
I think this probably looks something like
class Tweak {
virtual bool prepare(const Selection&); // as now, if false then we can't
use this tweak
struct Case { std::string title; json::Object Data };
vector<Case> cases(); // replaces title()
Expected<Effect> apply(json::Object Data);
}
// enumeration = prepare() + cases()
// application = prepare() + apply()
Maybe it makes sense to drop prepare() and have tweaks factor out any common
logic between cases & apply in whatever way suits them.
---
Anyway this is obviously yet another big yak-shave and this patch isn't the
place to discuss it, but I wanted to brain dump a bit. Let's talk next week.
Like I said we can go ahead with the approach in this patch, but I want to
understand the long-term.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99540/new/
https://reviews.llvm.org/D99540
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
