sammccall added a comment.
Herald added a project: clang-tools-extra.
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 at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:1
+//===--- ImplementAbstract.cpp -----------------------------------*-
C++-*-===//
+//
----------------
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?)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:8
+//===----------------------------------------------------------------------===//
+
+#include "refactor/Tweak.h"
----------------
A file comment here would be a great place to describe the functionality and
design choices.
e.g.
```
// A tweak to override inherited virtual methods in a class.
//
// Currently, supports overriding all pure-virtual methods at once (i.e.
implement abstract).
// It would be nice to add per-method overriding actions, but the Tweak
interface can't do this today.
```
================
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 =
----------------
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
:-))
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:46
+
+bool areAnyBasesDependent(const CXXRecordDecl &RD) {
+ for (const CXXBaseSpecifier &Base : RD.bases()) {
----------------
how is this different from the hasAnyDependentBases check in isClassOK?
This is called & checked in a lot of places, and makes its way into the return
value of various functions. It would be nice to validate upfront instead (and
you do appear to do that)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:61
+/// Detects if there are any non overridden methods declared in \p RD.
+bool detectPureMethodsImpl(
+ const CXXRecordDecl &RD,
----------------
It's hard to follow the logic of this function because the actual query logic
is combined with recursive AST traversal, early-returns, in-out params etc.
Moreover it's hard to understand its relationship to the other functions here,
and it's not clear how to generalize/share the logic (e.g. to support different
code actions for all pure functions together, all virtual functions
individually etc).
I'd suggest building a data structure first by walking the class hierarchy, and
then querying it:
```
struct VirtualMethods {
// All virtual methods anywhere in the hierarchy of the class T.
DenseMap<const CXXMethodDecl *, MethodInfo> Methods;
struct MethodInfo {
CXXMethodDecl *OverriddenBy = nullptr;
AccessSpecifier EffectiveAccess = AS_public; // in T
};
};
```
This seems straightforward and cheap-enough to build, and you can easily query
it for what we want here (methods not declared in T itself that are pure and
not overridden). Later if we want to emit separate code actions for each
virtual method, it's easy to reuse for that too.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:138
+/// if any of the bases are dependent, otherwise false.
+bool collectPureMethodsImpl(
+ const CXXRecordDecl &Record,
----------------
the return value here should be the results, not whether the bases are
dependent!
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:200
+const CXXRecordDecl *getSelectedRecord(const Tweak::Selection &Inputs,
+ Optional<CXXBaseSpecifier> *BaseSpec) {
+ const SelectionTree::Node *Node = Inputs.ASTSelection.commonAncestor();
----------------
I don't think restricting to the methods of the selected base-specifier is
worth the effort here and elsewhere.
For this to be useful, we need:
- the class to have two bases
- they each have virtual methods
- the user wants to implement one but not the other
- and the user is targeting the code action on the base spec
This is rare/obscure and we'd be better spending our complexity budget
elsewhere.
Instead, just treating the base specifier as part of the class seems enough.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:223
+/// virtual methods.
+bool isClassOK(const CXXRecordDecl &RecordDecl) {
+ if (!RecordDecl.isThisDeclarationADefinition())
----------------
suggest also checking that isBraceRange() is valid and its end point (`}`) is a
file ID not a macro.
That way we can guarantee that the find-insertion-point step never fails: if
there's any problem, just insert before `}`.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:240
+ AccessSpecifier Access;
+ unsigned char AfterPriority = 0;
+};
----------------
this is not used outside getInsertionPoints so shouldn't be in the return value
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:243
+
+// This is a little hacky because EndLoc of a decl doesn't include
+// the semi-colon.
----------------
why not insert before decls instead of after them? Start locations are pretty
reliable.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:249
+ return D.getEndLoc().getLocWithOffset(1);
+ if (auto Next = Lexer::findNextToken(D.getEndLoc(), SM, LO)) {
+ if (Next->is(tok::semi))
----------------
we generally try to avoid running the lexer.
You shouldn't need it at all I think, by inserting before, but TokenBuffer is
preferred if so.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:262
+SmallVector<InsertionDetail, 3>
+getInsertionPoints(const CXXRecordDecl &R, ArrayRef<bool> ShouldIncludeAccess,
+ const SourceManager &SM, const LangOptions &LO) {
----------------
this function seems more complex than necessary because it's trying to handle
all 3 access levels at once.
just take the access spec as a param, return the location, and have the caller
call several times?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:262
+SmallVector<InsertionDetail, 3>
+getInsertionPoints(const CXXRecordDecl &R, ArrayRef<bool> ShouldIncludeAccess,
+ const SourceManager &SM, const LangOptions &LO) {
----------------
sammccall wrote:
> this function seems more complex than necessary because it's trying to handle
> all 3 access levels at once.
> just take the access spec as a param, return the location, and have the
> caller call several times?
seems a bit cleaner to leave SourceLocation wrangling to the caller that's
dealing with edits, and just return a `decl_iterator` to insert before.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ImplementAbstract.cpp:301
+ if (Result.empty()) {
+ auto Access = R.isClass() ? AS_private : AS_public;
+ if (ShouldIncludeAccess[Access]) {
----------------
seems surprising to handle this as a special case, rather than just keeping
track of the effective access as we go.
What about:
```
AccessSpec CurAccess = class ? private : public;
decl_iterator Best;
enum {None, Bad, Good} Quality = None;
for (I = decls_begin(), E = decls_end(); I != E) {
Decl &D = *I++;
if (D is AccessSpecDecl)
CurAccess = D.getAccess();
if (CurAccess == TargetAccess) {
auto NewQuality = isa<CXXMethodDecl, AccessSpecDecl> ? Good : Bad;
if (NewQuality >= Quality) {
Quality = NewQuality;
Best = I;
}
}
}
if (Quality == None) {
Best = decls_end();
MustInsertSpecifier = true;
}
```
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
