Page MenuHomePhabricator

[clang-move] Add some options allowing to add old/new.h to new/old.h respectively.
ClosedPublic

Authored by hokein on Nov 22 2016, 5:19 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 78853.Nov 22 2016, 5:19 AM
hokein retitled this revision from to [clang-move] Add some options allowing to add old/new.h to new/old.h respectively..
hokein updated this object.
hokein added a reviewer: ioeric.
hokein added a subscriber: cfe-commits.
ioeric added inline comments.Nov 22 2016, 5:46 AM
clang-move/ClangMove.cpp
309 ↗(On Diff #78853)

How do you handle the case where a whole file is copied?

416 ↗(On Diff #78853)

This function is not very straight-forward to me so need some comments on what and why.

424 ↗(On Diff #78853)

Please check this in main. Otherwise, the Finder will still be run without any matcher.

594 ↗(On Diff #78853)

This variable seems pretty ugly. Can you just use MovedDecl.SM in the loop instead. It is used two times anyway.

608 ↗(On Diff #78853)

What does this check mean? Please comment. It seems weird to rely on a variable that is set in a loop as a condition.

Also, can you just do this to save some indentation?

if (!SM) return;
...
613 ↗(On Diff #78853)

We might want to do what include-fixer does to compute the best path to put in #include, e.g. we'd want #include "llvm/XXX.h" instead of #include "llvm/include/llvm/XXX.h"

clang-move/ClangMove.h
56 ↗(On Diff #78853)

additional seems redundant.

137 ↗(On Diff #78853)

You might want to explain what this is for and why it is needed.

hokein updated this revision to Diff 78874.Nov 22 2016, 7:52 AM
hokein marked 8 inline comments as done.

Address review comments.

clang-move/ClangMove.cpp
309 ↗(On Diff #78853)

Cases where a whole file is moved are handled in other functions, not in this function.

416 ↗(On Diff #78853)

Added a description in the method declaration.

613 ↗(On Diff #78853)

It might make sense, but it will require non-trivial change of this patch. Add a FIXME.

ioeric added inline comments.Nov 22 2016, 8:07 AM
clang-move/ClangMove.cpp
604 ↗(On Diff #78874)

Maybe call this FileAndReplacements instead of It, which is a bit confusing when reading the code below.

611 ↗(On Diff #78874)

Maybe add a comment here indicating that this replacement *will* be cleaned up at the end.

618 ↗(On Diff #78874)

Do we need to do something (e.g. error handling) instead of just continue? If continue is allowed, please add comment explaining why.

619 ↗(On Diff #78874)

Just use SI->second below since ID is only used once.

646 ↗(On Diff #78874)

Maybe just

std::string OldHeaderInclude = Spec.NewDependOnOld  ? "#include \"" + Spec.OldHeader + "\"\n" : ":";
clang-move/tool/ClangMoveMain.cpp
67 ↗(On Diff #78874)

s/depends/will depend/

This makes it clearer IMO. Same below.

unittests/clang-move/ClangMoveTests.cpp
435 ↗(On Diff #78874)

Also check that new does not depend on old?

458 ↗(On Diff #78874)

Also check that old does not depend on new?

hokein updated this revision to Diff 79043.Nov 23 2016, 1:25 AM
hokein marked 8 inline comments as done.

Update comments.

ioeric accepted this revision.Nov 23 2016, 2:00 AM
ioeric edited edge metadata.

Lg.

unittests/clang-move/ClangMoveTests.cpp
424 ↗(On Diff #79043)

Is this indented correctly? or just phabricator bug?

This revision is now accepted and ready to land.Nov 23 2016, 2:00 AM
hokein updated this revision to Diff 79046.Nov 23 2016, 2:06 AM
hokein marked an inline comment as done.
hokein edited edge metadata.

Fix code indentation.

This revision was automatically updated to reflect the committed changes.