[PATCH] D153152: Adds tweak to add declarations for pure virtuals
Robert Schneider via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 18 07:39:54 PDT 2023
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
More information about the cfe-commits
mailing list