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