This is an archive of the discontinued LLVM Phabricator instance.

[clangd] DefineOutline won't copy virtual specifiers on methods
ClosedPublic

Authored by njames93 on Mar 1 2020, 2:59 PM.

Details

Summary

The define out of line refactor tool previously would copy the virtual, override and final specifier into the out of line method definition.
This results in malformed code as those specifiers aren't allowed outside the class definition.

Diff Detail

Event Timeline

njames93 created this revision.Mar 1 2020, 2:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2020, 2:59 PM
njames93 edited the summary of this revision. (Show Details)Mar 1 2020, 3:03 PM
njames93 added a project: Restricted Project.
njames93 updated this revision to Diff 247533.Mar 1 2020, 4:54 PM
  • Extend to add virtual and final support
njames93 updated this revision to Diff 247535.Mar 1 2020, 5:14 PM
  • Made the replacements macro safe
njames93 updated this revision to Diff 247538.Mar 1 2020, 5:35 PM
  • Fix assertion when no Attrs are present
hokein edited reviewers, added: kadircet; removed: ilya-biryukov.Mar 2 2020, 12:08 AM

Thanks for working on this!

A few comments on macro handling and coding style. Apart from that mostly needs more testing.

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
216

nit:

auto DelAttr = [&](const Attr* A) { /* do magic */};
if(auto *OA = FD->getAttr<OverrideAttr>())
  DelAttr(OA);
if(auto *FA = ...)
219

can you rather use auto AttrToken = TB.getSpelledTokens(TB.getExpandedTokens(Attr.getRange())); throughout this part.
It can provide you with token range as well.

220

can you add some test cases for this branch ? In theory it should be ok to drop this if full expansion is just the attr, i.e.

#define FNL final
struct A { virtual void foo() FNL {} };

but should possibly fail in :

#define MACRO foo() final
struct A { virtual void MACRO {} };
236

nit: I believe QualifierInsertions needs to be renamed now, maybe something like DeclarationCleanups ?

244

again could you please add tests checking this case?

246

you would rather want to go over spelled tokens, as expandedtokens might not exist in the source code. (it looks like the usage above, not related to this patch, is also broken. No need to fix that one I'll try to prepare a fix, but patches welcome.)

TokBuf.spelledForExpanded(TokBuf.expandedTokens(SpecRange))

247

nit: use early exits, i.e:

if(Tok.kind() != tok::kw_virtual)
  continue;
``
249

same argument as above.

260

you can use Tok.range(SM) instead

clang-tools-extra/clangd/unittests/TweakTests.cpp
2113

can you also add a test case for final override/override final

njames93 retitled this revision from [clangd] DefineOutline removes `override` specified from overridden methods. to [clangd] DefineOutline won't copy virtual specifiers on methods.Mar 2 2020, 3:03 AM
njames93 edited the summary of this revision. (Show Details)
njames93 updated this revision to Diff 247601.Mar 2 2020, 4:35 AM
njames93 marked 8 inline comments as done.
  • Refactor and extra test cases

I'm not entirely sure how to get the spelledTokens working in a good macro safe way?

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
220

it's not a great idea refactoring functions with MACRO attributes, we can never know if there are side effects based on different definitions due to things like build configs.

I'm not entirely sure how to get the spelledTokens working in a good macro safe way?

I don't really follow this comment, could you elaborate? TB.expandedTokens always refer to a subset of the pre-processed token stream, so they can contain non-spelled tokens therefore clangd should never try to operate on those tokens. For example:

#define FOO(X) int X;
FOO(y);

pre-processed token stream will only contain int y which doesn't exist in the source code at all. TB.spelledForExpanded tries to map those expanded range back to spelling (to FOO(y) for example if you've got the full range int y), which is safe to operate on.
if there's no direct mapping between selected range and source code then it returns None, so you should bail out in such cases.

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
220

well that's definitely one way to go, but while designing this code action we decided user should know better and clangd currently provides this code action in cases involving macros, see DefineOutlineTest.HandleMacros and it would be inconsistent with rest of the behavior if it bailed out in half of the cases and kept working for the rest.

Also this case is even safer than the rest, since it is only trying to drop those qualifiers and not duplicate them in the definition side.

244

sorry, i still don't see any cases with multiple virtual keywords.

njames93 marked 2 inline comments as done.Mar 2 2020, 5:28 AM
njames93 added inline comments.
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
220

Thanks for explaining that for me, I'll give it a go and see how the test cases are handled

244

Ah I thought you meant a test case just containing virtual which there is, I'll get the second one added.
Should I even support multiple virtual decls, https://godbolt.org/z/kGbF22 clang treats this as a warning, but for gcc its an error.

kadircet added inline comments.Mar 2 2020, 5:49 AM
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
244

If compiler has an error parsing those, we'll be on the safe side since AST will be broken for that node and we'll most likely bail out long before we reach here.

If compiler is happy with those, and produces just warnings, deleting one vs deleting multiple shouldn't make much difference in the code path(just a matter of breaking or not), but would make action more resilient so I would go for deleting multiple, since it won't have any side effects the user will still see the compiler warning in the header(declaration location).

njames93 updated this revision to Diff 247620.Mar 2 2020, 6:27 AM
  • Macro handling more in line with rest of clangd
njames93 updated this revision to Diff 247621.Mar 2 2020, 6:30 AM
njames93 marked 3 inline comments as done.
  • added virtual virtual
njames93 marked 4 inline comments as done.Mar 2 2020, 6:30 AM
njames93 updated this revision to Diff 247640.Mar 2 2020, 7:14 AM
  • Fixed clang tidy warning, wont fix format as it's contradicting with the style of the rest of the function
kadircet added inline comments.Mar 2 2020, 7:32 AM
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
231

let's use syntax::Token::range(SM, AttrTokens->front(), AttrTokens->back())... instead

253

what about setting Any to true below this check and just breaking here instead? e.g.

bool Any = false;
for(...) {
  ....
  if (!Spelling) break;
  Any = true;
}
if (!Any) appendToErrors
261

why assert on this and not delete the whole Spelling that produced the virtual keyword ?

e.g.

#define STUPID_MACRO(X) virtual
struct F {
STUPID_MACRO(BLA BLA) void foo();
}
kadircet added a comment.EditedMar 2 2020, 7:36 AM
  • Fixed clang tidy warning, wont fix format as it's contradicting with the style of the rest of the function

I believe the formatting linter messages results from the fact that you are finishing the structs with a } on a newline, it should be right next to expected output, without any newlines in between.

  • Fixed clang tidy warning, wont fix format as it's contradicting with the style of the rest of the function

I believe the formatting linter messages results from the fact that you are finishing the structs with a } on a newline, it should be right next to expected output, without any newlines in between.

The linter messages are actually to do with the no new line after the { and bad indentation inside the braced init list, however that's how the rest look.

  • Fixed clang tidy warning, wont fix format as it's contradicting with the style of the rest of the function

I believe the formatting linter messages results from the fact that you are finishing the structs with a } on a newline, it should be right next to expected output, without any newlines in between.

The linter messages are actually to do with the no new line after the { and bad indentation inside the braced init list, however that's how the rest look.

yes it just suggests a formatting, but the formatting you want is dropping the new line just before } and hopefully that should also make linter happy.

njames93 updated this revision to Diff 247766.Mar 2 2020, 5:15 PM
  • Tweaked format
njames93 updated this revision to Diff 247779.Mar 2 2020, 6:37 PM
  • Addressed comments
kadircet accepted this revision.Mar 2 2020, 11:28 PM

Thanks, LGTM with a few small comments, please address those before landing.

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
244

double negation is bad(this already looks confusing, sounds like we start in an errorful state?) I think we should rather have HadErrors

261

sorry for not explicitly asking in the previous one, could you also add a test for this case?

clang-tools-extra/clangd/unittests/TweakTests.cpp
2175

you also need to delete the comma just before the }

This revision is now accepted and ready to land.Mar 2 2020, 11:28 PM
This revision was automatically updated to reflect the committed changes.
njames93 marked 6 inline comments as done.Mar 3 2020, 2:17 AM