This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Rearrange relocation codes in natural order. NFC.
ClosedPublic

Authored by ikudrin on Nov 27 2015, 7:08 AM.

Diff Detail

Event Timeline

ikudrin updated this revision to Diff 41306.Nov 27 2015, 7:08 AM
ikudrin retitled this revision from to [ELF/AArch64] Rearrange relocations to follow the documentation order. NFC..
ikudrin updated this object.
ikudrin added reviewers: ruiu, davide.
ikudrin added a project: lld.
ikudrin added a subscriber: llvm-commits.
ruiu edited edge metadata.Nov 27 2015, 7:12 AM

The original order looked random, but the new order also looks somewhat random. Should we instead order them alphabetically? Also, "follow the order in the spec" strategy does not work if some relocations are defined in a separate document (such as TLS relocations.)

ELF/Target.cpp
957

Actually I'd remove all these "// no overflow check needed." comments as that's obvious from code.

In D15045#297899, @ruiu wrote:

The original order looked random, but the new order also looks somewhat random. Should we instead order them alphabetically? Also, "follow the order in the spec" strategy does not work if some relocations are defined in a separate document (such as TLS relocations.)

OK. Alphabetical order also seems convenient to me.

ikudrin updated this revision to Diff 41313.Nov 27 2015, 8:26 AM
ikudrin retitled this revision from [ELF/AArch64] Rearrange relocations to follow the documentation order. NFC. to [ELF/AArch64] Rearrange AArch64 relocations in alphabetical order. NFC..
ikudrin edited edge metadata.
  • Rearranged alphabetically.
ruiu added a comment.Nov 27 2015, 9:15 AM

Please do this for all architectures.

emaste added a subscriber: emaste.Nov 27 2015, 10:46 AM
In D15045#297899, @ruiu wrote:

Should we instead order them alphabetically?

I'd certainly prefer alpha order rather than an arbitrary order that's the result of adding new ones at the end. But I wonder if there's a reasonable grouping that's lost in enforcing alpha order? Even with some relocations in separate documents we could produce a reasonable grouping - follow the order in the main spec, followed by the TLS spec for example.

ikudrin updated this revision to Diff 41389.Nov 30 2015, 5:10 AM
ikudrin retitled this revision from [ELF/AArch64] Rearrange AArch64 relocations in alphabetical order. NFC. to [ELF] Rearrange relocation codes in natural order. NFC..
ikudrin added reviewers: hfinkel, grimar.
  • Rearranged most of relocation codes within Target.cpp.

I have to note that I'm not sure that the new order is better than previous one for all the targets.

ruiu added a comment.Nov 30 2015, 9:09 AM

Is this alphabetical order?

ELF/Target.cpp
746–753

Keep the original style. (I know this style is a bit controversial, but at least I don't want to change that in this patch.)

In D15045#298613, @ruiu wrote:

Is this alphabetical order?

I meant this one: Natural sort order.

ELF/Target.cpp
746–753

Oops, clang-formatted unintentionally.

ruiu accepted this revision.Nov 30 2015, 10:30 AM
ruiu edited edge metadata.

LGTM

Usually relevant relocations share the same prefix, so they are grouped together in the natural order. I'm not sure which is considered standard, natural order or asciibetical order, but I prefer natural order, since "PC8, PC16, PC32, PC64" seems more natural to me than "PC16, PC32, PC64, PC8".

This revision is now accepted and ready to land.Nov 30 2015, 10:30 AM
davide accepted this revision.Nov 30 2015, 10:36 AM
davide edited edge metadata.

Fine to me as well then.

This revision was automatically updated to reflect the committed changes.