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

[Robert Schneider via Phabricator via cfe-commits]

robot added a comment.

I've pushed some WIP change to inform that I'm still working on this topic :) 
Not sure if I also need to submit a comment to publish what I've marked as 
"done" via checkboxes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Robert Schneider via Phabricator via cfe-commits]

robot updated this revision to Diff 556283.
robot marked 3 inline comments as done.
robot added a comment.
Herald added a project: clang.

WIP - several improvements

I thought it might be a good idea to give an update even though this is
still work in progress.

- Don't print `virtual` for added declarations
- Some rework of the declaration generation
- Add title with preview
- Make tweak only available if it adds anything
- Mostly fix multiple inheritance
- Disable tweak on forward declarations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h
  clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/include/clang/AST/CXXInheritance.h
  clang/include/clang/AST/PrettyPrinter.h
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -873,13 +873,13 @@
raw_ostream &OS) {
   if (T->hasTrailingReturn()) {
 OS << "auto ";
-if (!HasEmptyPlaceHolder)
+if (!HasEmptyPlaceHolder && Policy.ParenthesizeFunctionName)
   OS << '(';
   } else {
 // If needed for precedence reasons, wrap the inner part in grouping parens.
 SaveAndRestore PrevPHIsEmpty(HasEmptyPlaceHolder, false);
 printBefore(T->getReturnType(), OS);
-if (!PrevPHIsEmpty.get())
+if (!PrevPHIsEmpty.get() && Policy.ParenthesizeFunctionName)
   OS << '(';
   }
 }
@@ -903,7 +903,7 @@
 void TypePrinter::printFunctionProtoAfter(const FunctionProtoType *T,
   raw_ostream &OS) {
   // If needed for precedence reasons, wrap the inner part in grouping parens.
-  if (!HasEmptyPlaceHolder)
+  if (!HasEmptyPlaceHolder && Policy.ParenthesizeFunctionName)
 OS << ')';
   SaveAndRestore NonEmptyPH(HasEmptyPlaceHolder, false);
 
Index: clang/include/clang/AST/PrettyPrinter.h
===
--- clang/include/clang/AST/PrettyPrinter.h
+++ clang/include/clang/AST/PrettyPrinter.h
@@ -77,7 +77,7 @@
 PrintCanonicalTypes(false), PrintInjectedClassNameWithArguments(true),
 UsePreferredNames(true), AlwaysIncludeTypeForTemplateArgument(false),
 CleanUglifiedParameters(false), EntireContentsOfLargeArray(true),
-UseEnumerators(true) {}
+UseEnumerators(true), ParenthesizeFunctionName(true) {}
 
   /// Adjust this printing policy for cases where it's known that we're
   /// printing C++ code (for instance, if AST dumping reaches a C++-only
@@ -304,6 +304,21 @@
   /// enumerator name or via cast of an integer.
   unsigned UseEnumerators : 1;
 
+  /// Whether to wrap function names in function types in parentheses.
+  ///
+  /// This flag determines whether function types will printed with parentheses surrounding the name:
+  ///
+  /// \code
+  /// void (funcName)(int);
+  /// \endcode
+  ///
+  /// or without parentheses:
+  ///
+  /// \code
+  /// void funcName(int);
+  /// \endcode
+  unsigned ParenthesizeFunctionName : 1;
+
   /// Callbacks to use to allow the behavior of printing to be customized.
   const PrintingCallbacks *Callbacks = nullptr;
 };
Index: clang/include/clang/AST/CXXInheritance.h
===
--- clang/include/clang/AST/CXXInheritance.h
+++ clang/include/clang/AST/CXXInheritance.h
@@ -303,57 +303,78 @@
   void replaceAll(UniqueVirtualMethod Overriding);
 };
 
-/// A mapping from each virtual member function to its set of
-/// final overriders.
+/// A mapping from each virtual member function to its set of final overriders.
 ///
-/// Within a class hierarchy for a given derived class, each virtual
-/// member function in that hierarchy has one or more "final
-/// overriders" (C++ [class.virtual]p2). A final overrider for a
-/// virtual function "f" is the virtual function that will actually be
-/// invoked when dispatching a call to "f" through the
-/// vtable. Well-formed classes have a single final overrider for each
-/// virtual function; in abstract classes, the final overrider for at
-/// least one virtual function is a pure virtual function. Due to
-/// multiple, virtual inheritance, it is possible for a class to have
-/// more than one final overrider. Although this is an error (per C++
-/// [

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D153152#4617391 , @robot wrote:

>> so you could replace the logic that prints the return type + arg list with 
>> `printType(functype, class, methodname)` (from AST.h). That should pick up 
>> noexcept etc.
>
> I've tried it and it's not obvious to me how to do it properly: Since 
> `printType` wants to print the _function type_, it will parenthesize the 
> placeholder (methodname):
>
>   void (foo)()
>
> I've tried adding a flag to `PrintingPolicy` to avoid parenthesizing the 
> name, but I wonder if adjusting a "type printer" to print a function 
> declaration is a clean design. Next problem I ran into is parameters: A type 
> printer does not need to concern itself with parameter names nor default 
> arguments:
>
>   void foo(int x = 0) = 0;
>   void foo(int) override;
>
> Shall I continue to pursue that path? Shall I adjust the type printer to be 
> able to print parameter names and default arguments?

Sorry, it does sound like I sent you down a bad path.
You *could* (no need for an option for parens, here we can detect that the 
placeholder isValidAsciiIdentifier), but entirely possible more things are 
broken still.

If you prefer the approach with copying the tokens, adding heuristics to patch 
them up as needed is maybe no worse than the alternatives.

(BTW, I don't think we want to copy default arguments - it's certainly not 
necessary, and not helpful in common cases where the function is only called 
through the base)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Robert Schneider via Phabricator via cfe-commits]

robot added a comment.

> so you could replace the logic that prints the return type + arg list with 
> `printType(functype, class, methodname)` (from AST.h). That should pick up 
> noexcept etc.

I've tried it and it's not obvious to me how to do it properly: Since 
`printType` wants to print the _function type_, it will parenthesize the 
placeholder (methodname):

  void (foo)()

I've tried adding a flag to `PrintingPolicy` to avoid parenthesizing the name, 
but I wonder if adjusting a "type printer" to print a function declaration is a 
clean design. Next problem I ran into is parameters: A type printer does not 
need to concern itself with parameter names nor default arguments:

  void foo(int x = 0) = 0;
  void foo(int) override;

Shall I continue to pursue that path? Shall I adjust the type printer to be 
able to print parameter names and default arguments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Robert Schneider via Phabricator via cfe-commits]

robot marked an inline comment as done.
robot added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:58
+
+  const llvm::ArrayRef Tokens =
+  TokBuf.expandedTokens(MethodDeclRange);

sammccall wrote:
> robot wrote:
> > sammccall wrote:
> > > 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
> > My original idea was to take the AST node, clone it, remove the 
> > pure-specifier, and print that. To me, that seemed like the obvious 
> > (object-oriented) way how a tweak would manipulate or generate text.
> > Alas, I was rather stumped that there didn't seem to be support for this 
> > already, and it was rather hard to do. I gave up when default arguments 
> > came into play.
> > 
> > I've got the idea of manipulating the tokens from the "extract function" 
> > tweak (DefineOutline.cpp / getFunctionSourceCode).
> > 
> > I've tried the reprint function and it does not copy `noexcept`. That is 
> > simple to fix but my guess is there are more issues. The idea of token 
> > manipulation was to rather copy too much than too little. I have no 
> > intuition how many issues could arise from that :)
> > 
> > When I try to naively remove the `virtual` I currently print (`virtual void 
> > foo() override;`) in the token manipulation approach by skipping the first 
> > tokens, I end up skipping any attributes that precede the `virtual` as 
> > well. So I wrote a test for that, and reprint also doesn't copy attributes 
> > at the moment. It is not obvious to me if attributes should be copied, e.g. 
> > `[[noreturn]]`.
> > 
> > I have a slight preference for my original approach but it's probably too 
> > complicated if this is the only use case. Therefore I'll just implement 
> > whatever looks most promising to you.
> Yeah, I think extract function copies tokens because there could be anything 
> in the selected code, and there are certain constructs clang doesn't print 
> well, it's unclear what to do in the presence of macros etc. Those concerns 
> don't apply here I think.
> 
> > My original idea was to take the AST node, clone it, remove the 
> > pure-specifier, and print that. To me, that seemed like the obvious 
> > (object-oriented) way how a tweak would manipulate or generate text.
> 
> Yes, unfortunately this approach doesn't work because the AST isn't designed 
> to be mutated, and carries not just syntactic information but also semantic 
> invariants.
> (There was an effort to produce a separate representation of the syntax, but 
> it was never finished)
> 
> > I've tried the reprint function and it does not copy noexcept
> 
> True. It looks like the noexcept is part of the functionprototype, so you 
> could replace the logic that prints the return type + arg list with 
> `printType(functype, class, methodname)` (from AST.h). That should pick up 
> noexcept etc.
> 
> > It is not obvious to me if attributes should be copied, e.g. [[noreturn]].
> 
> No, I don't think attributes should be copied. They're not required to 
> override, and whether they semantically belong there depends on the 
> particular attribute - we can leave this up to the user.
> Yeah, I think extract function copies tokens because there could be anything 
> in the selected code, and there are certain constructs clang doesn't print 
> well, it's unclear what to do in the presence of macros etc. Those concerns 
> don't apply here I think.

Sorry, wrong tweak name (but right file/function). I meant "define outline". It 
also needs to massage the declaration since it needs to remove those parts 
which are not allowed in an outlined function definition, like `virtual` and 
`static`.

Anyway, I'll try adjusting `reprint()` next week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:58
+
+  const llvm::ArrayRef Tokens =
+  TokBuf.expandedTokens(MethodDeclRange);

robot wrote:
> sammccall wrote:
> > 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
> My original idea was to take the AST node, clone it, remove the 
> pure-specifier, and print that. To me, that seemed like the obvious 
> (object-oriented) way how a tweak would manipulate or generate text.
> Alas, I was rather stumped that there didn't seem to be support for this 
> already, and it was rather hard to do. I gave up when default arguments came 
> into play.
> 
> I've got the idea of manipulating the tokens from the "extract function" 
> tweak (DefineOutline.cpp / getFunctionSourceCode).
> 
> I've tried the reprint function and it does not copy `noexcept`. That is 
> simple to fix but my guess is there are more issues. The idea of token 
> manipulation was to rather copy too much than too little. I have no intuition 
> how many issues could arise from that :)
> 
> When I try to naively remove the `virtual` I currently print (`virtual void 
> foo() override;`) in the token manipulation approach by skipping the first 
> tokens, I end up skipping any attributes that precede the `virtual` as well. 
> So I wrote a test for that, and reprint also doesn't copy attributes at the 
> moment. It is not obvious to me if attributes should be copied, e.g. 
> `[[noreturn]]`.
> 
> I have a slight preference for my original approach but it's probably too 
> complicated if this is the only use case. Therefore I'll just implement 
> whatever looks most promising to you.
Yeah, I think extract function copies tokens because there could be anything in 
the selected code, and there are certain constructs clang doesn't print well, 
it's unclear what to do in the presence of macros etc. Those concerns don't 
apply here I think.

> My original idea was to take the AST node, clone it, remove the 
> pure-specifier, and print that. To me, that seemed like the obvious 
> (object-oriented) way how a tweak would manipulate or generate text.

Yes, unfortunately this approach doesn't work because the AST isn't designed to 
be mutated, and carries not just syntactic information but also semantic 
invariants.
(There was an effort to produce a separate representation of the syntax, but it 
was never finished)

> I've tried the reprint function and it does not copy noexcept

True. It looks like the noexcept is part of the functionprototype, so you could 
replace the logic that prints the return type + arg list with 
`printType(functype, class, methodname)` (from AST.h). That should pick up 
noexcept etc.

> It is not obvious to me if attributes should be copied, e.g. [[noreturn]].

No, I don't think attributes should be copied. They're not required to 
override, and whether they semantically belong there depends on the particular 
attribute - we can leave this up to the user.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Robert Schneider via Phabricator via cfe-commits]

robot planned changes to this revision.
robot marked 3 inline comments as done.
robot added a comment.

I'm working on the comments, but it will take a while.




Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:58
+
+  const llvm::ArrayRef Tokens =
+  TokBuf.expandedTokens(MethodDeclRange);

sammccall wrote:
> 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
My original idea was to take the AST node, clone it, remove the pure-specifier, 
and print that. To me, that seemed like the obvious (object-oriented) way how a 
tweak would manipulate or generate text.
Alas, I was rather stumped that there didn't seem to be support for this 
already, and it was rather hard to do. I gave up when default arguments came 
into play.

I've got the idea of manipulating the tokens from the "extract function" tweak 
(DefineOutline.cpp / getFunctionSourceCode).

I've tried the reprint function and it does not copy `noexcept`. That is simple 
to fix but my guess is there are more issues. The idea of token manipulation 
was to rather copy too much than too little. I have no intuition how many 
issues could arise from that :)

When I try to naively remove the `virtual` I currently print (`virtual void 
foo() override;`) in the token manipulation approach by skipping the first 
tokens, I end up skipping any attributes that precede the `virtual` as well. So 
I wrote a test for that, and reprint also doesn't copy attributes at the 
moment. It is not obvious to me if attributes should be copied, e.g. 
`[[noreturn]]`.

I have a slight preference for my original approach but it's probably too 
complicated if this is the only use case. Therefore I'll just implement 
whatever looks most promising to you.



Comment at: clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp:153
+  llvm::StringLiteral kind() const override {
+return CodeAction::QUICKFIX_KIND;
+  }

sammccall wrote:
> 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"?
For reference:
rust-analyser shows a "Quick Fix" in VSCode for implementing the functions in a 
trait.
The go extension shows a "Rewrite" (which is an entirely different action?) in 
VSCode for filling a structure object (inserting code which explicitly 
initializes each member with its null value).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 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();
+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(&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/Declar

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

[Robert Schneider via Phabricator via cfe-commits]

robot updated this revision to Diff 532575.
robot added a comment.

Remove left-over test file of other ptch from CMakeLists.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp
@@ -0,0 +1,665 @@
+//===-- DeclarePureVirtualsTests.cpp *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestTU.h"
+#include "TweakTesting.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TWEAK_TEST(DeclarePureVirtuals);
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerOnBaseSpecifier) {
+  EXPECT_AVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : ^p^u^b^l^i^c^ ^M^y^B^a^s^e {
+};
+
+class MyDerived2 : ^M^y^B^a^s^e {
+};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerOnClass) {
+  EXPECT_AVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class ^M^y^D^e^r^i^v^e^d: public MyBase {^
+// but not here, see AvailabilityTriggerInsideClass
+^};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerInsideClass) {
+  // TODO: this should actually be available but I don't know how to implement
+  // it: the common node of the selection returns the TU, so I get no
+  // information about which class we're in.
+  EXPECT_UNAVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : public MyBase {
+^
+};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, UnavailabilityNoBases) {
+  EXPECT_UNAVAILABLE(R"cpp(
+class ^N^o^D^e^r^i^v^e^d^ ^{^
+^};
+  )cpp");
+}
+
+// TODO: should the tweak available if there are no pure virtual functions and
+// do nothing? or should it be unavailable?
+
+TEST_F(DeclarePureVirtualsTest, SinglePureVirtualFunction) {
+  const char *Test = R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : pub^lic MyBase {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : public MyBase {
+virtual void myFunction() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, MultipleInheritanceFirstClass) {
+  const char *Test = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : pub^lic MyBase1, public MyBase2 {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, public MyBase2 {
+virtual void myFunction1() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, MultipleInheritanceSecondClass) {
+  const char *Test = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, pub^lic MyBase2 {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, public MyBase2 {
+virtual void myFunction2() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, SingleInheritanceMultiplePureVirtualFunctions) {
+  const char *Test = R"cpp(
+class MyBase {
+  virtual void myFunction1() = 0;
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : pub^lic MyBase {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase {
+  virtual void myFunction1() = 0;
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase {
+virtual void myFunction1() override;
+virtual void myFunction2() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, SingleInheritanceMixedVirtualFunctions) {
+  const char *Test = R"cpp(
+class MyBase {
+  virtual void myFunction1() = 0;
+  virtual void myFunction2();
+};
+
+class MyDerived : pub^lic MyBase {
+};
+  )cpp";
+
+  const char *Expected

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

[Robert Schneider via Phabricator via cfe-commits]

robot updated this revision to Diff 532199.
robot added a comment.

Remove left-over file of other patch from CMakeLists.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp
@@ -0,0 +1,665 @@
+//===-- DeclarePureVirtualsTests.cpp *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestTU.h"
+#include "TweakTesting.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TWEAK_TEST(DeclarePureVirtuals);
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerOnBaseSpecifier) {
+  EXPECT_AVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : ^p^u^b^l^i^c^ ^M^y^B^a^s^e {
+};
+
+class MyDerived2 : ^M^y^B^a^s^e {
+};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerOnClass) {
+  EXPECT_AVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class ^M^y^D^e^r^i^v^e^d: public MyBase {^
+// but not here, see AvailabilityTriggerInsideClass
+^};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerInsideClass) {
+  // TODO: this should actually be available but I don't know how to implement
+  // it: the common node of the selection returns the TU, so I get no
+  // information about which class we're in.
+  EXPECT_UNAVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : public MyBase {
+^
+};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, UnavailabilityNoBases) {
+  EXPECT_UNAVAILABLE(R"cpp(
+class ^N^o^D^e^r^i^v^e^d^ ^{^
+^};
+  )cpp");
+}
+
+// TODO: should the tweak available if there are no pure virtual functions and
+// do nothing? or should it be unavailable?
+
+TEST_F(DeclarePureVirtualsTest, SinglePureVirtualFunction) {
+  const char *Test = R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : pub^lic MyBase {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : public MyBase {
+virtual void myFunction() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, MultipleInheritanceFirstClass) {
+  const char *Test = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : pub^lic MyBase1, public MyBase2 {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, public MyBase2 {
+virtual void myFunction1() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, MultipleInheritanceSecondClass) {
+  const char *Test = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, pub^lic MyBase2 {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, public MyBase2 {
+virtual void myFunction2() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, SingleInheritanceMultiplePureVirtualFunctions) {
+  const char *Test = R"cpp(
+class MyBase {
+  virtual void myFunction1() = 0;
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : pub^lic MyBase {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase {
+  virtual void myFunction1() = 0;
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase {
+virtual void myFunction1() override;
+virtual void myFunction2() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, SingleInheritanceMixedVirtualFunctions) {
+  const char *Test = R"cpp(
+class MyBase {
+  virtual void myFunction1() = 0;
+  virtual void myFunction2();
+};
+
+class MyDerived : pub^lic MyBase {
+};
+  )cpp";
+
+  const char *Expected = R

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

[Robert Schneider via Phabricator via cfe-commits]

robot added a comment.

This change is not done, I need some advice:

- I don't understand why there are two levels of data structures in an entry of 
the final overriders map, hence I can't properly name the variables nor do I 
fully understand what I'm doing there.
- It's not clear if the tweak should be available if there are no pure virtual 
functions.
- It's not clear if we need to handle the case that there are multiple final 
overriders, or what we should do in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153152

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Robert Schneider via Phabricator via cfe-commits]

robot created this revision.
robot added a reviewer: sammccall.
Herald added subscribers: ChuanqiXu, kadircet, arphaman.
Herald added a project: All.
robot requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

This tweak searches the current or a selected base class for functions
which are still pure virtual in the current class, and adds function
declarations (overriders) for them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153152

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/DeclarePureVirtuals.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/tweaks/DeclarePureVirtualsTests.cpp
@@ -0,0 +1,665 @@
+//===-- DeclarePureVirtualsTests.cpp *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestTU.h"
+#include "TweakTesting.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TWEAK_TEST(DeclarePureVirtuals);
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerOnBaseSpecifier) {
+  EXPECT_AVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : ^p^u^b^l^i^c^ ^M^y^B^a^s^e {
+};
+
+class MyDerived2 : ^M^y^B^a^s^e {
+};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerOnClass) {
+  EXPECT_AVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class ^M^y^D^e^r^i^v^e^d: public MyBase {^
+// but not here, see AvailabilityTriggerInsideClass
+^};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, AvailabilityTriggerInsideClass) {
+  // TODO: this should actually be available but I don't know how to implement
+  // it: the common node of the selection returns the TU, so I get no
+  // information about which class we're in.
+  EXPECT_UNAVAILABLE(R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : public MyBase {
+^
+};
+  )cpp");
+}
+
+TEST_F(DeclarePureVirtualsTest, UnavailabilityNoBases) {
+  EXPECT_UNAVAILABLE(R"cpp(
+class ^N^o^D^e^r^i^v^e^d^ ^{^
+^};
+  )cpp");
+}
+
+// TODO: should the tweak available if there are no pure virtual functions and
+// do nothing? or should it be unavailable?
+
+TEST_F(DeclarePureVirtualsTest, SinglePureVirtualFunction) {
+  const char *Test = R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : pub^lic MyBase {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase {
+  virtual void myFunction() = 0;
+};
+
+class MyDerived : public MyBase {
+virtual void myFunction() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, MultipleInheritanceFirstClass) {
+  const char *Test = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : pub^lic MyBase1, public MyBase2 {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, public MyBase2 {
+virtual void myFunction1() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, MultipleInheritanceSecondClass) {
+  const char *Test = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, pub^lic MyBase2 {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase1 {
+  virtual void myFunction1() = 0;
+};
+
+class MyBase2 {
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase1, public MyBase2 {
+virtual void myFunction2() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(DeclarePureVirtualsTest, SingleInheritanceMultiplePureVirtualFunctions) {
+  const char *Test = R"cpp(
+class MyBase {
+  virtual void myFunction1() = 0;
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : pub^lic MyBase {
+};
+  )cpp";
+
+  const char *Expected = R"cpp(
+class MyBase {
+  virtual void myFunction1() = 0;
+  virtual void myFunction2() = 0;
+};
+
+class MyDerived : public MyBase {
+virtual void myFunction1() override;
+virtual void myFunction2() override;
+};
+  )cpp";
+
+  EXPECT_EQ(apply(Test), Expected);
+}
+
+TEST_F(Decl