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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
