Add a tweak that will create declarations for all its base classes unimplemented pure virtual methods.
Depends on D95231
Differential D94942
[clangd] Add tweak for implementing abstract class njames93 on Jan 18 2021, 8:41 PM. Authored by
Details
Diff Detail
Event TimelineComment Actions
Comment Actions 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).
Comment Actions 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.
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.
As code completion can help implement just one virtual method, not much is gained by offering to implement methods one by one.
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.
I couldn't even rush this and get it ready before Tuesday :) Definitely not gonna make the 12 cut, even in experimental state. Comment Actions If cursor is over one of the base specifiers, offer to implement only the pure methods from that base class.
Comment Actions
Comment Actions Tweak the prepare method to just check for methods that need implementations, this saves some work that could be deferred to the apply method. Comment Actions Thanks for working on this and sorry for the long delay! I also think it's really useful, I hope we can simplify the impl a little and land it.
Comment Actions One thing I noticed was that you need to be over the base class or overriding class identifier in order to get the code action. For some editors like Visual Studio Code this is less noticeable but for others like VIM it is more noticeable as you need to know it's there. Is there a way to trigger the tweak when it is anywhere in the class? I have been investigating how to do it but I am still learning a lot about the clang AST and the internals of clangd. What do other people think? Might not be worth adding to this patch as it would over-complicate an already complicated patch. Comment Actions @njames93 I had completely forgotten about this when I attempted the same thing early this year. Some thoughts based on that:
Comment Actions I stopped working on this a while ago but happy to resume (I've cherry picked this to the custom clangd build I use and it definitely has some value) |
I'd consider calling this OverrideVirtual.cpp. I think we'll want to support method-by-method triggering in future, and it would share most of the implementation.
(We don't have the infrastructure today, but there are certainly more cases where we want to offer alternate tweaks from the same "class". @kadircet maybe this is relevant to bazel build fixing?)