Page MenuHomePhabricator

[llvm-objcopy] Accept --long-option but not -long-option
ClosedPublic

Authored by MaskRay on Apr 8 2019, 9:34 PM.

Details

Summary

llvm-{objcopy,strip} (and many other LLVM binary utilities) accept
cl::opt style -long-option as well as many short options (e.g. -p -S
-x). People who use them as replacement of GNU binutils often use the
grouped option syntax (POSIX Utility Conventions), e.g. -Sx => -S -x,
-Wd => -W -d, -sj.text => -s -j.text

There is ambiguity if a long option starts with the character used by a
short option. Drop the support for -long-option to resolve the ambiguity.

This divergence from other utilities is accepted (other utilities
continue supporting -long-option).
https://lists.llvm.org/pipermail/llvm-dev/2019-April/131786.html

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Apr 8 2019, 9:34 PM
Herald added a project: Restricted Project. · View Herald Transcript
jhenderson accepted this revision.Apr 10 2019, 4:14 AM

The behaviour change looks good, in the sense that it does what you set out to do. I don't have any real objections to it aside from the fact that it is different from how LLVM tools typically work, but I personally also find it odd the LLVM tools work with a single-dash! I'd like others to comment on this though before giving the thumbs up, particularly @jakehehrlich as the code owner.

This revision is now accepted and ready to land.Apr 10 2019, 4:14 AM
rupprecht accepted this revision.Apr 15 2019, 1:39 AM

LGTM. I much prefer only - short and -- long options, except for lld where unfortunately there's a precedent of accepting both.

except for lld where unfortunately there's a precedent of accepting both.

ld.lld -d -ont -b lame -lld -t -hats -v -ery -understandablec -ompatibilit -yissues

grimar added a subscriber: grimar.Apr 16 2019, 11:32 AM

I feel like we didn't get a solid conclusion here but that the general consensus is that this change is more ideal. I think the argument that this is incompatible with merged short options and that that might have compatibility issues is a strong argument personally. The other not-quite-conclusion-but-leaning-that-way seems to be that we can't really make this same choice for all binaries. Do we feel like the conclusion of the llvm-dev post supports yet further fragmenting the tools? I don't know and I'm concerned about making that choice. We have to way tool consistency with GNU compatibility here and that's a hard choice. I'll add an email later to see if we can flesh this out more.

I feel like we didn't get a solid conclusion here but that the general consensus is that this change is more ideal. I think the argument that this is incompatible with merged short options and that that might have compatibility issues is a strong argument personally. The other not-quite-conclusion-but-leaning-that-way seems to be that we can't really make this same choice for all binaries. Do we feel like the conclusion of the llvm-dev post supports yet further fragmenting the tools? I don't know and I'm concerned about making that choice. We have to way tool consistency with GNU compatibility here and that's a hard choice. I'll add an email later to see if we can flesh this out more.

In the thread https://lists.llvm.org/pipermail/llvm-dev/2019-April/131786.html some people expressed that the single-dash -long-option seems unusual. Note, cl::Grouping is not used in utilities other than these binary utilities. Some people said the policy can be set per-command basis.

Can we fix llvm-objcopy now?

jakehehrlich accepted this revision.Apr 25 2019, 2:26 PM

Eh I guess this is fine. I'm kind of 50/50 on this myself. I don't think you'll break any major team or code base I know about but I could be wrong. I don't really like fragmenting the standard used across these tools but grouped options increases compatibility so I think on balance I support this.

It would be helpful to convert all of the listed tools over as well for consistency

It would be helpful to convert all of the listed tools over as well for consistency

Thank you! I'll find some easier ones to convert. llvm-objcopy is the starter as it is the youngest :)

MaskRay updated this revision to Diff 196779.Apr 25 2019, 7:01 PM
MaskRay edited the summary of this revision. (Show Details)

Rebase. Update description. Handle --allow-broken-links

This revision was automatically updated to reflect the committed changes.