[PATCH] D82923: introducing llvm-libtool-darwin

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 16 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:
> 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


================
Comment at: llvm/docs/CommandGuide/llvm-libtool-darwin.rst:40
+:program:`llvm-libtool-darwin` exits with a non-zero exit code if there is an error.
+Otherwise, it exits with code 0.
+
----------------
jhenderson wrote:
> It might also be worth adding a "See Also" section for `llvm-ar` since the two tools are somewhat related.
Makes sense, thanks!


================
Comment at: llvm/test/tools/llvm-libtool-darwin/help-message.test:10
+
+# UNKNOWN-ARG: Unknown command line argument '{{-+}}abcabc'
----------------
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.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:25
+
+# DOUBLE-OUTPUT: for the --output option: must occur exactly one time!
----------------
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?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:22-23
+                                       cl::Required);
+static cl::alias OutputFileShort("o", cl::desc("Alias for -output"),
+                                 cl::aliasopt(OutputFile));
+
----------------
jhenderson wrote:
> A common thing you may not be aware of is that `cl::opt` aliases don't appear in the help text by default. I imagine this is not desirable for you, so you probably want to add a `cl::NotHidden` here (I think that's the right term anyway).
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82923