[PATCH] D82923: introducing llvm-libtool-darwin

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 7 inline comments as done.
sameerarora101 added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-libtool-darwin.rst:24-26
+.. option:: -help, -h
+
+  Display usage information and exit.
----------------
jhenderson wrote:
> sameerarora101 wrote:
> > jhenderson wrote:
> > > I suspect if you do `-help` you'll see one or two other options too. You'll probably want to include `-help-list` here at least, and might need to hide some other options. Could you post (out-of-line) what the full help text is, please?
> > here is the full help text from cmd line:
> > 
> > ```
> > OVERVIEW: llvm-libtool-darwin
> > 
> > USAGE: llvm-libtool-darwin [options] <input files>
> > 
> > OPTIONS:
> > 
> > Color Options:
> > 
> >   --color             - Use colors in output (default=autodetect)
> > 
> > General options:
> > 
> >   -o                  - Alias for --output
> >   --output=<filename> - Specify output filename
> > 
> > Generic Options:
> > 
> >   --help              - Display available options (--help-hidden for more)
> >   --help-list         - Display list of available options (--help-list-hidden for more)
> >   --version           - Display the version of this program
> > ```
> > 
> > I have added description for `--help-list` and `--color` now as well
> Thanks. Some more comments:
> 
> 1) As this is a Darwin-inspired tool, we should use the standard option naming throughout. If I understand it correctly, this means single dashes rather than double.
> 2) You probably want to use the documentation for the various common options (help, version, color etc) used in the other tool documentation, for consistency. Take a look at e.g. llvm-objcopy or llvm-dwarfdump. In particular, I wouldn't report the "hidden" versions of the help options (they're hidden for a reason...).
> 3) Documentation should use full English grammar rules with leading caps and trailing full stops, like comments in the code.
Ok, I have replaced `--` with `-` and took help from `llvm-dwarfdump` for the common options.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test:1
+## This test checks that an error is thrown in case of invalid input/output args.
+
----------------
jhenderson wrote:
> jhenderson wrote:
> > On further reflection, perhaps it makes sense to combine this and basic.test into a single test. What do you think?
> Actually, ignore my previous comment, since basic.test is only short-term.
> 
> You'll probably want to add --static to these tests when you add support to that option to avoid any potential confusion.
Ya I agree, `basic.test` is temporary and I replace it with `create-static-lib.test` when adding the `-static` option.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82923/new/

https://reviews.llvm.org/D82923