Page MenuHomePhabricator

[clang-refactor] Apply source replacements
ClosedPublic

Authored by arphaman on Sep 29 2017, 6:47 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric added inline comments.Oct 3 2017, 12:22 AM
test/Refactor/tool-apply-replacements.cpp
6 ↗(On Diff #117133)

Why is the result class {?

tools/clang-refactor/ClangRefactor.cpp
319 ↗(On Diff #117133)

I think Changes or SourceChanges would be a clearer variable name.

330 ↗(On Diff #117133)

s/AllSourceReplacements/Changes or SourceChanges for clarity.

410 ↗(On Diff #117133)

s/Replacements/Changes/ ?

416 ↗(On Diff #117133)

I think we would also want to clean up by default in the future. Maybe add this to FIXME as well?

arphaman updated this revision to Diff 118042.Oct 6 2017, 11:07 AM
arphaman marked 5 inline comments as done.

Address review comments

test/Refactor/tool-apply-replacements.cpp
6 ↗(On Diff #117133)

Sorry, the test was broken. Fixed!

hokein added inline comments.Oct 9 2017, 2:16 AM
include/clang/Frontend/CommandLineSourceLoc.h
57 ↗(On Diff #118042)

Add a comment documenting what the first element and second element of the pair represent for (<Line, Column>? If I understand correctly).

60 ↗(On Diff #118042)

Would be clearer to document the valid format of the Str.

test/Refactor/tool-apply-replacements.cpp
3 ↗(On Diff #118042)

Maybe add one more case for testing -selection=%t.cp.cpp:6:7-6:15.

tools/clang-refactor/ClangRefactor.cpp
309 ↗(On Diff #118042)

Add override.

313 ↗(On Diff #118042)

The same, override.

412 ↗(On Diff #118042)

nit: missing a blank after open.

arphaman updated this revision to Diff 118471.Oct 10 2017, 1:52 PM
arphaman marked 6 inline comments as done.

Address review comments

ioeric accepted this revision.Oct 10 2017, 1:59 PM

lgtm

This revision is now accepted and ready to land.Oct 10 2017, 1:59 PM
hokein accepted this revision.Oct 11 2017, 1:05 AM

LGTM, let's check in it.

This revision was automatically updated to reflect the committed changes.