[PATCH] D153152: Adds tweak to add declarations for pure virtuals

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for working on this!

FWIW I had an old draft of this feature that may have interesting ideas: https://reviews.llvm.org/D122827



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:44
+    for (const auto &WhatIsThis :
+         Overrider.second) { // TODO: why is there a second level????
+      return !WhatIsThis.Method->isPure();
----------------
see the long comment on CXXFinalOverriderMap 
TL;DR: a derived class can inherit multiple times from the same base, each such subobject could potentially have a separate override


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:58
+
+  const llvm::ArrayRef<syntax::Token> Tokens =
+      TokBuf.expandedTokens(MethodDeclRange);
----------------
copying the tokens directly seems brittle, for example we shouldn't be emitting "virtual" but its's hard to avoid doing so.
(Also, surprising that getEndLoc() doesn't cover the `= 0`!)

D122827 has a reprint() function that seems to work


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:75
+// same as or derived from Start.
+std::string collectPureVirtualFuncOverrideDecls(
+    const CXXRecordDecl &Target, const CXXRecordDecl &Start,
----------------
this probably needs to be restructured somewhat as need to enumerate these in prepare() to be sure there are any, but don't want to pay to produce the strings


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:86
+  CXXFinalOverriderMap FinalOverriderMapOfTarget;
+  // If &Target == &Start then Target doesn't already have any
+  // overrides for functions that are pure in Start. The map remains empty,
----------------
FWIW, I'm not actually sure that the case of multiple bases classes each providing pure-virtual-methods of which you want to override only one set is common enough to be worth this extra complexity.




================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:139
+///   class Base    { virtual void foo() = 0; };
+///   class Derived {};
+///
----------------
please use `^^` markers underneath the characters where you expect the tweak to trigger


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:143
+///   class Base    { virtual void foo() = 0; };
+///   class Derived { virtual void foo() override; };
+class DeclarePureVirtuals : public Tweak {
----------------
no "virtual"


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:149
+  std::string title() const override {
+    return "Override pure virtual functions";
+  }
----------------
what do you think about listing the functions we'll define?

Especially in the case where there's only one.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:153
+  llvm::StringLiteral kind() const override {
+    return CodeAction::QUICKFIX_KIND;
+  }
----------------
I don't think this is a quick-fix, which should address a diagnostic or so.

Really there doesn't seem to be a standard kind that fits here. Maybe we should add a new constant "generate"?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:156
+
+  bool prepare(const Selection &Sel) override {
+    const SelectionTree::Node *const CommonAncestor =
----------------
it's important that we only offer the code action (i.e. prepare returns true) if there are actually methods to override.

This means actually enumerating the functions needs to happen in prepare rather than apply. (Which means it needs to be fast...)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:163
+    // maybe selected a class, in which case override functions of all bases
+    SelectedDerivedClass = CommonAncestor->ASTNode.get<CXXRecordDecl>();
+    if (SelectedDerivedClass) {
----------------
this should only trigger on the class definition, not forward declarations


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:170
+    // bases's functions
+    const DeclContext &DC = CommonAncestor->getDeclContext();
+    SelectedDerivedClass = dyn_cast<CXXRecordDecl>(&DC);
----------------
this seems confusing: if you're going to walk up looking for a CXXBaseSpecifier, why not just do that in the first place?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:196
+      if (!Start) {
+        return llvm::createStringError(
+            llvm::inconvertibleErrorCode(),
----------------
this doesn't seem like a condition we should be surfacing to the user.
How do we get here? if it's e.g. because the base is dependent, then we should be returning false from prepare() in the appropriate dependent-code cases.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:212
+        SM, tooling::Replacements(tooling::Replacement(
+                SM, SelectedDerivedClass->getEndLoc(), 0, Additions)));
+  }
----------------
inserting at the end of the class isn't the most appropriate place.

we have InsertionPoint.h for this, see usage in D122827


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp:68
+// TODO: should the tweak available if there are no pure virtual functions and
+// do nothing? or should it be unavailable?
+
----------------
it should be unavailable
(this is important, availability triggers the "lightbulb" icon in vscode and is interpreted by users as "something to do here")


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp:591
+
+// This test should fail since MyBase is incomplete. No idea how to test that.
+// TEST_F(DeclarePureVirtualsTest, IncompleteBassClass) {
----------------
EXPECT_UNAVAILABLE, we shouldn't be offering the tweak


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152