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

[Sam McCall via Phabricator via cfe-commits]

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