This is an archive of the discontinued LLVM Phabricator instance.

[clang-move] Make it support both relative and absolute file path arguments.
ClosedPublic

Authored by hokein on Sep 26 2016, 9:21 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 72503.Sep 26 2016, 9:21 AM
hokein retitled this revision from to [clang-move] Make it support both relative and absolute file path arguments..
hokein updated this object.
hokein added a reviewer: ioeric.
hokein added a subscriber: cfe-commits.
hokein updated this revision to Diff 72613.Sep 27 2016, 1:26 AM

Rebase to master

ioeric added inline comments.Sep 27 2016, 1:59 AM
clang-move/ClangMove.cpp
33 ↗(On Diff #72613)

This doesn't seems to be good error handling... maybe also print an error message in case this fails in release binary.

39 ↗(On Diff #72613)

There are two versions of this. Please add comments about the differences.

43 ↗(On Diff #72613)

Same as above. Error handling.

62 ↗(On Diff #72613)

To generalize the function, maybe also remove dots in AbsoluteFilePath?

78 ↗(On Diff #72613)

Looks like MakeAbsolutePath(SM, SearchPath) can be done in addIncludes since you are also passing SM.

109 ↗(On Diff #72613)

I think it would be easier to read if you put OriginalRunningDirectory before OldHeader .

clang-move/ClangMove.h
63 ↗(On Diff #72613)

This function could use more comments. What are those parameters and what are they for?

64 ↗(On Diff #72613)

Should SearchPath be absolute or relative?

88 ↗(On Diff #72613)

s/saves/save the/

hokein updated this revision to Diff 72619.Sep 27 2016, 2:55 AM
hokein marked 8 inline comments as done.

Address review comments.

clang-move/ClangMove.cpp
62 ↗(On Diff #72613)

AbsoluteFilePath has been removed dots, and the matcher here is only used internally by clang-move, so I'd prefer the current way here, if we meet the generalized case in the future, I will be happy to extend it.

ioeric accepted this revision.Sep 30 2016, 1:48 AM
ioeric edited edge metadata.

Lg with nits.

clang-move/ClangMove.cpp
36 ↗(On Diff #72619)

s/warning/Warning/

51 ↗(On Diff #72619)

s/warning/Warning

test/clang-move/Inputs/database_template.json
3 ↗(On Diff #72619)

Maybe decorate "test_dir" as "$test_dir" or "%test_dir" or similar so that others would know it is gonna replaced.

This revision is now accepted and ready to land.Sep 30 2016, 1:48 AM
hokein updated this revision to Diff 73426.Oct 4 2016, 1:40 AM
hokein marked 3 inline comments as done.
hokein edited edge metadata.

Add clang-move tool in test/CMakeLists.

This revision was automatically updated to reflect the committed changes.