This is an archive of the discontinued LLVM Phabricator instance.

[clang-rename] exit code-related bugfix and code cleanup
ClosedPublic

Authored by omtcyf0 on Jul 7 2016, 7:34 AM.

Details

Summary

This patch does the following:

  • enforces proper formatting for few files (i.e. deals with 80 linewidth violations and few other things)
  • ensures '\n' chars are passed to the output streams instead of "\n" strings
  • fixes a bug caused by calling cl::PrintHelpMessage(), which occasionally calls exit(0), so that exit(1) (which is right after cl::PrintHelpMessage line) becomes dead code

Diff Detail

Repository
rL LLVM

Event Timeline

omtcyf0 updated this revision to Diff 63069.Jul 7 2016, 7:34 AM
omtcyf0 retitled this revision from to [clang-rename] exit code-related bugfix and code cleanup.
omtcyf0 updated this object.
omtcyf0 added reviewers: alexfh, klimek, bkramer, ioeric.
omtcyf0 added a subscriber: cfe-commits.
vmiklos added a subscriber: vmiklos.Jul 7 2016, 8:08 AM

Can you please postpone the cleanup till http://reviews.llvm.org/D21814 is reviewed? The two patches conflict with each other, I fear. Thanks. :-)

bkramer added inline comments.Jul 7 2016, 9:15 AM
clang-rename/USRLocFinder.h
38 ↗(On Diff #63069)

In LLVM we usually have only one space before a // comment. was this clang-formatted with google style?

omtcyf0 updated this revision to Diff 63093.Jul 7 2016, 10:47 AM
omtcyf0 marked an inline comment as done.
omtcyf0 added inline comments.
clang-rename/USRLocFinder.h
38 ↗(On Diff #63069)

Oops. Yes, seems like my machine had Google as the default style, instead of the LLVM code style by default, which I usually have.

Can you please postpone the cleanup till http://reviews.llvm.org/D21814 is reviewed? The two patches conflict with each other, I fear. Thanks. :-)

Ignore this, that one won't land as-is after all.

alexfh added inline comments.Jul 9 2016, 1:15 PM
clang-rename/USRLocFinder.cpp
63 ↗(On Diff #63093)

This would look better as a range-based for loop.

76 ↗(On Diff #63093)

clang::tooling:fixit::getText may be a good alternative here.

omtcyf0 updated this revision to Diff 63418.Jul 10 2016, 1:30 AM
omtcyf0 marked an inline comment as done.Jul 10 2016, 1:33 AM
omtcyf0 added inline comments.
clang-rename/USRLocFinder.cpp
73–75 ↗(On Diff #63418)

Not sure whether I know how to address this one.

clang::tooling:fixit::getText returns text of a whole AST Node, while here we only want to get text of the first token that AST Node has. I.e. 'foo' vs. 'foo(0)'.

omtcyf0 updated this revision to Diff 63463.Jul 11 2016, 12:22 AM

Add new test, which ensures clang-rename failure if new name is not passed.

Does it look good now?

alexfh accepted this revision.Jul 14 2016, 12:14 AM
alexfh edited edge metadata.

LG with one comment.

clang-rename/USRLocFinder.cpp
73–76 ↗(On Diff #63463)

Fair enough. I didn't get the intent of the code then.

clang-rename/USRLocFinder.h
33 ↗(On Diff #63463)

Use llvm style for names: USR, Decl.

This revision is now accepted and ready to land.Jul 14 2016, 12:14 AM
omtcyf0 updated this revision to Diff 63931.Jul 14 2016, 12:43 AM
omtcyf0 edited edge metadata.
omtcyf0 marked an inline comment as done.

Thanks, @alexfh!

Can you please land it?

Thanks, @alexfh!

Can you please land it?

It's not easy to do from the phone ;) Please ask someone else (Ben?).

bkramer edited edge metadata.Jul 14 2016, 2:47 AM

on it ...

This revision was automatically updated to reflect the committed changes.