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

Fri Jan 22 06:36:18 PST 2021

njames93 added a comment.

In D94942#2515049 <https://reviews.llvm.org/D94942#2515049>, @kadircet wrote:

> 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).

There was no high level discussions on my end. I'm just a guy on my own, who makes what I like, I don't even work in software. Though if there is a public place where people seem to discuss these I'd like to be pointed in the direction(discord seems to be mainly used for user support and clangd-dev has only had one relevant post in the last 6 months). Right now seems that GitHub issues and here are my best bets.

> - 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)

I'm still unsure of the best way to go about that, Trouble with just defining empty methods is we will undoubtedly generate code that can't compile (without warnings), Mostly due to methods not having return statements or if we try and fix that, how do you implement the return when the function needs to return a reference.

> - 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.

As code completion can help implement just one virtual method, not much is gained by offering to implement methods one by one.
Then there's the issue of say if a class has 50 virtual methods, Having 50 different refactoring show up in the UI is likely going to be some issue.
There's a discussion <https://github.com/microsoft/language-server-protocol/issues/1164> of adding a refactoring related methods to LSP which would enable a more interactive experience. There hasn't been an agreed design yet though. If that does make it through it would definitely extend the usefulness of this. Presenting a Interface to the user where they could choose exactly what methods they want to implement as well as letting them override already implemented virtual functions.

> - 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.

Fair point. Regarding the base specifier, Currently it doesn't offer the tweak when over the base specifier, I may be inclined to tweak behaviour so If you are over the base specifier, then only offer to implement pure virtual methods from that base class. Although rather annoyingly, The selection considers the `public|private|protected|virtual` part of a base specifier as part of the derived class, Instead of being part of the base specifier. I'm gonna push a separate patch to address that though.

> - 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.

I couldn't even rush this and get it ready before Tuesday :) Definitely not gonna make the 12 cut, even in experimental state.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94942