Page MenuHomePhabricator

[clang-rename] add basic vim integration
ClosedPublic

Authored by omtcyf0 on Jul 7 2016, 4:25 AM.

Details

Summary

This patch introduces basic Vim integration for clang-rename tool.

For setup reference see clang-rename/tool/clang-rename.py

Diff Detail

Repository
rL LLVM

Event Timeline

omtcyf0 updated this revision to Diff 63050.Jul 7 2016, 4:25 AM
omtcyf0 retitled this revision from to [clang-rename] add basic vim integration.
omtcyf0 updated this object.
omtcyf0 added reviewers: alexfh, klimek.
omtcyf0 added a reviewer: ioeric.
omtcyf0 added a subscriber: cfe-commits.
bkramer added inline comments.Jul 7 2016, 5:02 AM
clang-rename/tool/ClangRename.cpp
39 ↗(On Diff #63050)

This looks unrelated.

clang-rename/tool/clang-rename.py
14 ↗(On Diff #63050)

The comment should say that you have to save the file before running the tool.

40 ↗(On Diff #63050)

Maybe add a FIXME to input the unsaved file via stdin like we do for clang-format?

47 ↗(On Diff #63050)

stdin is pipe but you never write to it? Is that intentional?

48 ↗(On Diff #63050)

Should we output stderr in case the tool fails?

omtcyf0 updated this revision to Diff 63054.Jul 7 2016, 5:18 AM
omtcyf0 marked 5 inline comments as done.
bkramer accepted this revision.Jul 7 2016, 5:19 AM
bkramer edited edge metadata.

looks good. Still no commit access? ;)

This revision is now accepted and ready to land.Jul 7 2016, 5:19 AM

Yup. FeelsBadMan.

One note about that header removal: how do I do it then? I thought attaching such changes to a random patch is not a problem. Otherwise there will be some one-line patches for such things, won't they?

clang-rename/tool/ClangRename.cpp
39 ↗(On Diff #63050)

Right, but I'm not sure one-line patches are welcome.

Is it not alright to put such into random patches? Otherwise creating one-liner...

ioeric added inline comments.Jul 7 2016, 5:25 AM
clang-rename/tool/clang-rename.py
12 ↗(On Diff #63054)

Maybe a different key binding so that it doesn't conflict with include-fixer's suggested key binding? ,cr for clang-rename maybe?

33 ↗(On Diff #63054)

Redirect error messages to stderr so that they are highlighted in vim?

bkramer added inline comments.Jul 7 2016, 5:28 AM
clang-rename/tool/ClangRename.cpp
39 ↗(On Diff #63050)

I'd do it on the next change to that file, but I can also include it in the patch this time. I generally dislike having completely unrelated changes in a patch and the unnecessary include here doesn't really hurt anyone.

alexfh added inline comments.Jul 7 2016, 5:34 AM
clang-rename/tool/clang-rename.py
12 ↗(On Diff #63054)

Also, "clang-include-fixer.py" should be updated.

omtcyf0 updated this revision to Diff 63057.Jul 7 2016, 5:34 AM
omtcyf0 edited edge metadata.
omtcyf0 marked 2 inline comments as done.
omtcyf0 marked an inline comment as done.
omtcyf0 added inline comments.
clang-rename/tool/clang-rename.py
13 ↗(On Diff #63057)

Aw, sure; sorry for that. That's an artifact from first version of the script, so this line was simply copy-pasted...

alexfh added inline comments.Jul 7 2016, 5:36 AM
clang-rename/tool/ClangRename.cpp
39 ↗(On Diff #63050)

> I'm not sure one-line patches are welcome.

Cleanup-only patches are fine.

alexfh accepted this revision.Jul 7 2016, 5:37 AM
alexfh edited edge metadata.

LG

clang-rename/tool/clang-rename.py
17 ↗(On Diff #63057)

s/,cf/,cr/

omtcyf0 updated this revision to Diff 63058.Jul 7 2016, 5:40 AM
omtcyf0 edited edge metadata.
omtcyf0 marked an inline comment as done.

@bkramer can you please land the patch?

This revision was automatically updated to reflect the committed changes.
kimgr added a subscriber: kimgr.Jul 7 2016, 8:09 AM

I only caught this typo after it was committed.

clang-tools-extra/trunk/clang-rename/tool/clang-rename.py
17–18

s/promted/prompted for/

@kimgr oops, sorry.

I'll send a cleanup patch.

Thanks for noticing.