[PATCH] D94942: [clangd] Add tweak for implementing abstract class

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Thanks this looks great, and something i've been longing for some time! But I got a couple of questions about the how the interaction is designed (sorry if I missed some high level discussions elsewhere, feel free to just point me in that direction).

- Why do we just declare methods, rather than defining them too (with dummy bodies) ? This would allow users to move function bodies out-of-line if they want to easily. Also it would enforce them to fill in the bodies, rather than forgetting/skipping some of the methods. (Even more maybe we should take the extra step and offer another action to declare the function inline and define it out-of-line, but I think that could be done iteratively if we see the need)
- Implementing all (pure) virtuals vs offering a code action for each possible method. It is unfortunate that our existing tweak infrastructure doesn't enable a single tweak to output multiple code actions, but I believe in this case we might achieve a whole lot better UX if we did offer implementing each method one by one, while possibly still suggesting implement "all" overrides or "only" pures. It is still useful in its current form ofcourse, as the user can manually change the declaration into a pure one, or delete it, but it sounds cumbersome if they only want to implement a small subset of all possible pure methods .
- Acting on non-pure virtuals too, i believe this is also a quite common use case that could benefit a lot of users. but this definitely increases the need for a code action per override.
- When to trigger? I suppose what you have in the code ATM makes sense (e.g. just on `[[class X]] : ... [[{]] [[}]]`) but it would be great to spell it out explicitly so that others have a chance to raise concerns. I don't think triggering within the body would be useful though, as users are likely to navigate within class body for various reasons, and spamming them with lots of codeactions at each cursor move is likely to be annoying.
- When to land this :) we are at the edge of a branch cut, and tweaks are a feature triggered automatically by editors. so any crashes in there are likely to make clangd useless (as they'll persist no matter what). so I believe we should either wait for release cut, and land this afterwards and fixing any crash reports we get until next release, or include it in current release while marking the tweak as hidden so that it will only annoy experimental users.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94942/new/

https://reviews.llvm.org/D94942