Details
Diff Detail
Event Timeline
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. |
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.
- 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.
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.) |
I meant this one: Natural sort order.
ELF/Target.cpp | ||
---|---|---|
746–753 | Oops, clang-formatted unintentionally. |
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".
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.)