Page MenuHomePhabricator

[llvm-objdump] - Cleanup the code. NFCI.
ClosedPublic

Authored by grimar on Sat, Jan 12, 7:00 AM.

Details

Summary

This is an initial NFC cleanup for the llvm-objdump code.

This patch:

  1. Renames things to match the official LLVM code style (lower case -> upper case).
  2. Removes few obviously excessive variables.
  3. Moves a few lines closer to the place of use, reorders the code a bit to simplify it, to avoid doing excessive returns and to avoid using 'else` after returns.
  • I tried to focus only on a llvm-objdump.h/llvm-objdump.cpp files. Few changes in the

MachODump.cpp and COFFDump.cpp are a result of llvm-objdump.h modification.

  • My intention wasn't about doing any significant code refactorings in this patch.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Sat, Jan 12, 7:00 AM

Normally, I wouldn't support whole-sale style changes like this, but the files have a weird mixture of styles, so I'm happy that we're normalising. Thanks!

llvm-objdump.cpp
571–572 ↗(On Diff #181440)

Any particular reason you've moved this one? The previous order I think was just as good.

1073–1074 ↗(On Diff #181440)

I prefer the old names (but LLVM-ified), i.e. RelCur and RelEnd.

1217–1218 ↗(On Diff #181440)

Similar comment to above: does this reordering give us anything?

1261–1262 ↗(On Diff #181440)

As above re. reordering.

1557–1558 ↗(On Diff #181440)

See above comments re. variable naming.

grimar marked an inline comment as done.Mon, Jan 14, 2:41 AM
grimar added inline comments.
llvm-objdump.cpp
571–572 ↗(On Diff #181440)

Previous order was by endianness. Isn't it a bit strange? I think grouping by address size is a much more common way in LLVM.

jhenderson added inline comments.Mon, Jan 14, 2:56 AM
llvm-objdump.cpp
571–572 ↗(On Diff #181440)

If you can confirm that address order is much more common, then this is fine. If it is not significantly more common, then I think we should avoid unnecessary change.

grimar marked an inline comment as done.Mon, Jan 14, 2:59 AM
grimar added inline comments.
llvm-objdump.cpp
571–572 ↗(On Diff #181440)

I am OK to omit this change from this patch. Other style inconsistencies that were addressed look a lot more critical to me.

jhenderson added inline comments.Mon, Jan 14, 3:12 AM
llvm-objdump.cpp
571–572 ↗(On Diff #181440)

I think so. I did a quick search, and its inconsistent across LLVM.

grimar updated this revision to Diff 181523.Mon, Jan 14, 3:15 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
This revision is now accepted and ready to land.Mon, Jan 14, 3:23 AM

LGTM.

Thanks! I'll hold this on until tomorrow to see if anyone has any comments/suggestions/objections.

This revision was automatically updated to reflect the committed changes.