This is an archive of the discontinued LLVM Phabricator instance.

[clang-move] Support moving function.
ClosedPublic

Authored by hokein on Nov 15 2016, 5:11 AM.

Event Timeline

hokein updated this revision to Diff 77983.Nov 15 2016, 5:11 AM
hokein retitled this revision from to [clang-move] Support moving function..
hokein updated this object.
hokein added a subscriber: cfe-commits.
ioeric added inline comments.Nov 16 2016, 2:07 AM
clang-move/ClangMove.cpp
414

s/ClassName/SymbolName/

417

I guess HasAnySymbolNames would be better when used in the matchers below.

test/clang-move/Inputs/function_test.h
1

Maybe add a test case where both classes and functions exist.

test/clang-move/move-function.cpp
9

Shouldn't there be empty lines around this line?

18

Why does this become one line after being moved?

Shouldn't it be:

template<typename T>
void h(T t) {}
hokein updated this revision to Diff 78160.Nov 16 2016, 3:12 AM
hokein marked 4 inline comments as done.

Add one more test and rename variables.

test/clang-move/move-function.cpp
9

There should be. FileCheck will skip empty lines and try to find next matching line here. Have added some \n checks.

18

formatAndApplyReplacements does this for us ,and this is actually LLVM code style...

ioeric accepted this revision.Nov 16 2016, 3:20 AM
ioeric edited edge metadata.

Lg with two nits.

clang-move/ClangMove.cpp
145

Maybe just inline this variable since this is only used once.

146

I'd assert(FD) instead.

This revision is now accepted and ready to land.Nov 16 2016, 3:20 AM
hokein updated this revision to Diff 78171.Nov 16 2016, 5:10 AM
hokein edited edge metadata.

Fix nits.

This revision was automatically updated to reflect the committed changes.