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