[PATCH] D82923: introducing llvm-libtool-darwin

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-libtool-darwin.rst:24-26
+.. option:: -help, -h
+
+  Display usage information and exit.
----------------
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.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/basic.test:1
+# This test checks that main exits normally (EC 0) for correct input/output args.
+
----------------
Nit: use '##' for comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/help-message.test:10
+
+# UNKNOWN-ARG: Unknown command line argument '{{-+}}abcabc'
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > I believe this should include `error:` as a prefix? Please add it, if so. Same applies in the other test.
> No, the output doesn't have `error:` as a prefix. Here is the output for passing `-abcabc`:
> ```
> llvm-libtool-darwin: Unknown command line argument '-abcabc'.  Try: './bin/llvm-libtool-darwin --help'
> ```
> 
> `error:` is also not there as a prefix for the other test.
Yuck, okay. Maybe something to look at another time, I guess.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:25
+
+# DOUBLE-OUTPUT: for the --output option: must occur exactly one time!
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > You probably want a test that shows that the expected passing cases also work (i.e. exit with code 0 for now). This could probably be a test file called `basic.test` for now. You can replace or expand the test as you add useful functionality. I'd expect the test to have cases for both one input and multiple inputs.
> Ok, I added `basic.test` where I simply run
> ```
> # RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
> # RUN: yaml2obj %S/Inputs/input2.yaml -o %t-input2.o
> 
> ## Pass single input:
> # RUN: llvm-libtool-darwin -o %t.lib %t-input1.o
> 
> ## Pass multiple inputs:
> # RUN: llvm-libtool-darwin -o %t.lib %t-input1.o %t-input2.o
> ```
> Is this sufficient? Or is there some other way I need to verify that the exit code was indeed 0?
That's sufficient, thanks. If a tool returns a non-zero exit code, lit will automatically fail unless `not` is prepended.


================
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.
+
----------------
On further reflection, perhaps it makes sense to combine this and basic.test into a single test. What do you think?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:22
+                                       cl::Required);
+static cl::alias OutputFileShort("o", cl::desc("Alias for --output"),
+                                 cl::aliasopt(OutputFile), cl::NotHidden);
----------------
Similar to my documentation comment, I'm okay with this using single-dash if it's more common on Darwin.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82923