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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:1
+//===--- ImplementAbstract.cpp -----------------------------------*- C++-*-===//
+//
----------------
sammccall wrote:
> 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?)
>(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?)

Yes we should hopefully have support for those in the near future!


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:25
+// FIXME: Have some way to control this, maybe in the config?
+constexpr bool DefineMethods = true;
+using MethodAndAccess =
----------------
sammccall wrote:
> I know you just added this, but I think it's better to declare only, and hope to compose with a "define method" code action.
> 
> Reasons:
>  - It's a lot of work to provide sensible defaults: `return {}` is clever, but unidiomatic for many return types.
>  - We can't produce bodies for all return types (e.g. classes with no easy constructor). So we'll produce a bunch of methods that don't compile, which is distracting.
>  - Inserting a dummy body that *does* compile places a burden on the user to keep track of it.
>  - Inserting *in-line* definitions doesn't really save much typing or much thinking
>  - code actions are a pretty simple interaction with few "options". Offering every permutation is unrealistic, and config doesn't seem like an ergonomic alternative. Our best hope IMO is combining sequential code actions.
>  - keeps the scope small, smaller code actions are easier to maintain
> 
> @kadircet do you find this compelling? (Don't want Nathan caught in the middle :-))
I agree 100%.

In addition to all of those, getting linker errors for missing bodies is a lot better than debugging arbitrary runtime misbehaviour due to a defaulted return type.

As a future note on the `define method` code action, i think rather than trying to generate a compiling definition we should construct a body like:
`return /*magic*/;` to ensure user gets some diagnostics (at least for non-void functions), that they can use to jump later on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94942