This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho][NFC] Add some new version options, plus a new option group for versions
ClosedPublic

Authored by gkm on Jan 18 2021, 7:00 PM.

Details

Reviewers
compnerd
int3
Group Reviewers
Restricted Project
Commits
rG0ef25cf558bf: [lld-macho][NFC] Add new option group for versions
Summary

Add some options present in ld64 but absent here: -tvos_version_min, watchos_version_min, bridgeos_version_min, and driverkit_version_min.
For sake of organization in the --help output, coalesce all version-control options into a new option group.

Diff Detail

Event Timeline

gkm created this revision.Jan 18 2021, 7:00 PM
gkm requested review of this revision.Jan 18 2021, 7:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2021, 7:00 PM
compnerd requested changes to this revision.Jan 19 2021, 8:55 AM
compnerd added a subscriber: compnerd.

This adds additional options while also adding a group for the versions. Is there a reason for the group to be added? Can you please either remove the newly added options or adjust the commit message? I think that there should be some reasoning given for the version group creation (at least, it isn't clear to me why the group is helpful/necessary, unlike in clang where the group can be used to silence warnings).

This revision now requires changes to proceed.Jan 19 2021, 8:55 AM
smeenai added a subscriber: smeenai.EditedJan 19 2021, 12:20 PM

We've been pretty liberal with our use of option groups, mostly to organize the help text. I agree that the commit message should reflect that there's some new version options being added as well (tvos, watchos, bridgeos, and driverkit, if I'm reading the diff right).

Sure, but if the reason for the group is to add structure to the help output, I think that is a reasonable thing to state in the commit message. The other options are ... well, either not meant to be part of the commit as per the commit message or the commit message should be updated to indicate that they are being added (and why they are part of structuring the help output, assuming that is the reason for the group). The changes requested are requests for clarification :)

gkm requested review of this revision.Jan 28 2021, 9:22 PM
gkm retitled this revision from [lld-macho][NFC] Add new option group for versions to [lld-macho][NFC] Add some new version options, plus a new option group for versions.
gkm edited the summary of this revision. (Show Details)
gkm edited the summary of this revision. (Show Details)

@compnerd, I updated the title and summary as requested.

int3 accepted this revision.Jan 28 2021, 11:38 PM
int3 added a subscriber: int3.
int3 added inline comments.
lld/MachO/Options.td
235

"version control" makes me think of, well, git and hg... how about "target version"?

273
compnerd accepted this revision.Jan 29 2021, 10:29 AM

+1 to @int3's suggestion about the help text.

This revision is now accepted and ready to land.Jan 29 2021, 10:29 AM
This revision was automatically updated to reflect the committed changes.
int3 added inline comments.Feb 1 2021, 9:17 AM
lld/MachO/Options.td
235

seems like you missed this