Page MenuHomePhabricator

[clangd] Define out-of-line availability checks
ClosedPublic

Authored by kadircet on Oct 21 2019, 9:31 AM.

Details

Summary

Initial availability checks for performing define out-of-line code
action, which is a refactoring that will help users move function/method
definitions from headers to implementation files.

Proposed implementation only checks whether we have an interesting selection,
namely function name or full function definition/body.

Diff Detail

Event Timeline

kadircet created this revision.Oct 21 2019, 9:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2019, 9:31 AM

the patch looks mostly good. would be interesting to see apply implementation.

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

nit: looks like we also a similar version in DefineInline? would be nice if we could share the implementation. I don't have a good idea where to put it, maybe add a FIXME?

45

I'm not sure this is useful in general: saying in .cc we have a single definition void foo() {}, after running the code action, we will end up with void foo(); void foo() {}.

If we want to do this, I think we may make it only available for inline class methods.

76

The bail-out here seems violating the above comment /// If current file is already an implementation file, tries to move the definition out-of-line. Basically, now we only allow moving the function definition to a .cc file.

79

I think we can use AST.getASTContext().getLangOpts().IsHeaderFile; to detect the header file rather than relying on the filename extension.

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

could you add a testcase where the method is inline? like

class Bar {
  void baz2() { return; }
}
kadircet updated this revision to Diff 226045.Oct 22 2019, 7:15 AM
kadircet marked 8 inline comments as done.
  • Address comments
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
30

I also had the same concern, but left it here since couldn't find a suitable please.
adding a fixme.

45

yes this was implying methods in my head, but forgot to spell it out. updating comment.

76

this was actually representing the state I wanted to arrive, but you are right it looks confusing, getting rid of the mention in the class documention.

hokein accepted this revision.Oct 23 2019, 5:30 AM
hokein added inline comments.
clang-tools-extra/clangd/unittests/TweakTests.cpp
1725

nit: please explicitly spell out the FileName, even the default value is TestTU.cpp.

1748

Is this needed? It is unclear to me why we need it.

This revision is now accepted and ready to land.Oct 23 2019, 5:30 AM
hokein added inline comments.Oct 24 2019, 1:58 AM
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
78

I think doesThisDeclarationHaveABody is probably a better choice here? isThisDeclarationADefinition returns true for defaulted methods like void Ctor() = default;, we'd need to disable the tweak for this case, could you add a test case for this?

hokein added inline comments.Oct 24 2019, 2:50 AM
clang-tools-extra/clangd/unittests/TweakTests.cpp
1756

Sorry for not spotting it earlier.

Moving out-of-line methods is different than general functions, void Bar::bar(); is illegal in C++ (C++ requires the out-of-line member declaration must be a definition). I think for this case, we could 1) delete the original method definition 2) disable the tweak. I slightly prefer 2) as out-of-line member definitions are rare in header files. WDYT?

kadircet marked 6 inline comments as done.Oct 25 2019, 12:31 AM
kadircet added inline comments.
clang-tools-extra/clangd/unittests/TweakTests.cpp
1748

yeah not anymore, I was checking for existence of implementation file in the first version. but moved it from prepare to apply, forgot to update the test.

1756

oopsy thanks for spotting this. yeah I agree with 2.

kadircet updated this revision to Diff 226381.Oct 25 2019, 12:31 AM
kadircet marked 2 inline comments as done.
  • Address comments
hokein accepted this revision.Oct 28 2019, 5:46 AM
hokein added inline comments.
clang-tools-extra/clangd/unittests/TweakTests.cpp
1749

nit: metohds => methods.

still lgtm.

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

nit: we have the isHeaderFile helper in the SourceCode.h, please use it (it is more precise).

kadircet updated this revision to Diff 231374.Nov 28 2019, 1:37 AM
kadircet marked 2 inline comments as done.
  • Address comments
  • Bail out on templated classes

Build result: pass - 60292 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

This revision was automatically updated to reflect the committed changes.