Added "move into source" helper
ClosedPublic

Authored by skalinichev on Feb 22 2016, 7:12 AM.

Details

Summary

This mostly uses the code from oldcpp. Which I cleaned up a bit and removed a lot of unused functionality. Still I had to introduce a couple of changes to the code due to e.g. inconsistencies in internal vs. function contexts with oldcpp.

Also while working on it I discovered a couple of related bugs in our codebase, which are included in this review, but will be landed in separated commits.

BUG: 358480
BUG: 355148

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
skalinichev updated this revision to Diff 2424.Feb 22 2016, 7:12 AM
skalinichev retitled this revision from to Added "move into source" helper.
skalinichev updated this object.
skalinichev edited the test plan for this revision. (Show Details)
skalinichev set the repository for this revision to R32 KDevelop.
skalinichev added a project: KDevelop.
skalinichev added subscribers: KDevelop, kdevelop-devel.
skalinichev updated this revision to Diff 2425.Feb 22 2016, 7:19 AM
kfunk added a subscriber: kfunk.Feb 22 2016, 7:46 AM

Just had a brief look

languages/clang/clangsupport.cpp
278

FYI: The reason this shortcut was changed:

commit bee81aa39f493733d411dfa7efaeb89720237ecc
Author: Olivier JG <olivier.jg@gmail.com>
Date: Fri Oct 9 12:24:53 2015 +0200

Fixes for rename action

Don't duplicate BasicRefactoring
Use KActionCollection::setDefaultShortcut to make it configurable
*Use a default shortcut that doesn't conflict with Klipper

Not sure what else to use, though. IMO we should put the refactoring actions into a context menu/popup we can invoke with a keyboard shortcut instead.

Something for a future commit.

languages/clang/codegen/simplerefactoring.cpp
59 ↗(On Diff #2425)

Early return?

62 ↗(On Diff #2425)

Ditto

109 ↗(On Diff #2425)

Nitpick: Remove newline

198 ↗(On Diff #2425)

QVector?

skalinichev updated this revision to Diff 2428.Feb 22 2016, 8:50 AM
skalinichev marked 5 inline comments as done.

Minor improvements

mwolff added a subscriber: mwolff.Feb 22 2016, 2:20 PM

overall really good work, thanks a lot Sergey! I was actually thinking of working on that the other day but got distracted. So excellent timing on your side :)

I have a bunch of nitpicks, but overall this is fabulous work already. I guess most of my nitpicks actually are valid in the old code paths as well, so you are not to blame here. I just want to get this cleaned up before we get it in for clang.

languages/clang/codegen/simplerefactoring.cpp
133 ↗(On Diff #2428)

afaik we have a "isHeader" check, no? If not, introduce it please

134 ↗(On Diff #2428)

the below should be moved into a helper function, "documentFinderHelpers::sourceForHeader(headerPath)"

204 ↗(On Diff #2428)
auto localDeclarations = funcCtx->localDeclarations();
signature.reserve(localDeclarations.size());

or even better, use std::transform with a back_inserter and a lambda for the conversion.

264 ↗(On Diff #2428)

shouldn't be required anymore afaik

languages/clang/codegen/simplerefactoring.h
38 ↗(On Diff #2428)

This name was (and still is) really ill-chosen. can you come up with something better? I'd go for ClangRefactoring.

languages/clang/codegen/sourcemanipulation.cpp
78

don't we have such a function elsewhere already, e.g. for the "update signature" assistant? could we share code?

233

uhm can't we use the source formatter to do this work for us?

269

align with line above again

307

early return if == continue; to decrease indent level

languages/clang/codegen/sourcemanipulation.h
49

align first const with line above

73

DUContextPointer?

75

ReferencedTopDUContext?

languages/clang/duchain/builder.cpp
563–564 ↗(On Diff #2437)

in upstream clang?

skalinichev marked 13 inline comments as done.

Address review issues

mwolff accepted this revision.Feb 23 2016, 10:58 AM
mwolff added a reviewer: mwolff.

lgtm, can't find anything obviously wrong. Fabulous work Sergey, many thanks!

This revision is now accepted and ready to land.Feb 23 2016, 10:58 AM
This revision was automatically updated to reflect the committed changes.
kfunk added a comment.Mar 4 2016, 2:26 PM

@skalinichev: Breaks unit tests on CI, please investigate:

13:32:11 QFATAL : TestAssistants::testMoveIntoSource(add-into-namespace) ASSERT: "!decls.isEmpty()" in file /home/jenkins/builds/kdevelop/stable-kf5-qt5/languages/clang/tests/test_assistants.cpp, line 560
13:32:11 FAIL! : TestAssistants::testMoveIntoSource(add-into-namespace) Received a fatal error.
13:32:11 Loc: [Unknown file(0)]
13:32:11 Totals: 35 passed, 1 failed, 0 skipped, 0 blacklisted
13:32:11 *** Finished testing of TestAssistants ***

https://build.kde.org/view/Kdevelop/job/kdevelop%205.0%20stable-kf5-qt5/183/PLATFORM=Linux,compiler=gcc/consoleFull