[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<syntax::Token> 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