This is an archive of the discontinued LLVM Phabricator instance.

[llvm-lipo] Add docs for llvm-lipo
ClosedPublic

Authored by alexander-shaposhnikov on May 30 2019, 3:18 PM.

Details

Summary

In this diff the file llvm-lipo.rst is added.

Test plan:
make -j8 sphinx
check that ./docs/html/CommandGuide/llvm-lipo.html is built correctly and looks okay.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 3:18 PM
Herald added a subscriber: arphaman. · View Herald Transcript

LGTM, but I'd wait for @mtrent.

docs/CommandGuide/llvm-lipo.rst
11 ↗(On Diff #202317)

Nit: wrapping seems a bit weird here?

mtrent added a comment.Jun 6 2019, 3:28 PM

I had trouble getting recommonmark.parser to install on my system, so I hacked at the patch to get the html and groff formatters to run. Should not be relevant to this review.

Please consider breaking apart the required commands from the optional options. This isn't important yet, but as soon as you add "-arch" you're going to have to deal with arguments that are optional and apply to more than one command, and arguments where one-and-only-one command is required. You can see an example in llvm-objdump.rst:

SYNOPSIS
     llvm-objdump [commands] [options] [filenames...]

DESCRIPTION
     The llvm-objdump utility prints stuff[...]

COMMANDS
     At least one of the following commands are required, and some commands can be combined with other commands:
[ ... ]

OPTIONS
     llvm-objdump supports the following options:
[ ... ]

Recent versions of the lipo man page also try to make this clear, although sometimes it's hard.

I rendered the CommandGuide as man and also as html, seemed ok.

docs/CommandGuide/llvm-lipo.rst
12 ↗(On Diff #202322)

Serial comma is helpful in lists. "object files,"

"Report some information" is somewhat vague. Consider "report architectural information".

Consider "program can create universal binaries from [MDT? Mach-O?] files, extract regular object files from universal binaries, and display architecture information about both universal and regular files."

26 ↗(On Diff #202322)

Only one arch is necessary. Consider -verify_arch <architecture 1> [<architecture 2> ...]

28 ↗(On Diff #202322)

Verify that the specified architecture is present by doing what? Best to call out the command's exit status here.

Is it obvious what "architecture" means here? How would you describe that to someone?

The -verify_arch command only works on a single file (the Apple man page suggests otherwise, but AFAICT the code has never supported that). Do you need to mention that here?

Address @mtrent comments,

the static html file (attached to my previous comments) contains some relative paths to various .css style sheets, so it needs to be placed into <your_build_dir>/docs/html/CommandGuide/ to be rendered correctly

alexander-shaposhnikov marked an inline comment as done.Jun 6 2019, 4:27 PM
alexander-shaposhnikov added inline comments.
docs/CommandGuide/llvm-lipo.rst
28 ↗(On Diff #202322)

>Is it obvious what "architecture" means here?
no, it's not, honestly, I don't have a good explanation appropriate for docs other than "supported architecture name" with some popular examples, would you like me to add these bits here ?

mtrent accepted this revision.Jun 7 2019, 7:54 AM

Thanks for making this!

docs/CommandGuide/llvm-lipo.rst
28 ↗(On Diff #202322)

Looks like the major llvm tools (clang, lldb) just refer to "architecture" and leave it at that. llvm-objdump says "see version for available architectures" but the list is weird/incomplete. Let's leave it.

The opportunity of the man page / CommandGuide is to give some additional context that one can't infer from the command usage / -help. Considering lipo's job, this may be the best place in llvm.org to explain what these things are. All that said, that task isn't urgent, and it can wait for the rest of the functionality to come in. Having this file in place will help with that.

This revision is now accepted and ready to land.Jun 7 2019, 7:54 AM
This revision was automatically updated to reflect the committed changes.