[PATCH] D82923: introducing llvm-libtool-darwin

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/index.rst:23
+   llvm-libtool-darwin
    llvm-link
    llvm-lib
----------------
Would you mind, whilst you are modifying this list, putting this list in alphabetical order? I think it makes more sense that way.


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


================
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.
+
----------------
It might also be worth adding a "See Also" section for `llvm-ar` since the two tools are somewhat related.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/help-message.test:4
+# RUN: llvm-libtool-darwin -h | FileCheck --check-prefix=LIBTOOL-USAGE %s --match-full-lines
+# RUN: llvm-libtool-darwin --help | FileCheck --check-prefix=LIBTOOL-USAGE %s --match-full-lines
+# RUN: not llvm-libtool-darwin -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
----------------
You might want to add a `-help` version here too.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/help-message.test:10
+
+# UNKNOWN-ARG: Unknown command line argument '{{-+}}abcabc'
----------------
I believe this should include `error:` as a prefix? Please add it, if so. Same applies in the other test.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:1
+## This test checks that an error is thrown in case of invalid argument(s).
+
----------------
I'd dedicate this test to specifically input/output file arguments. Perhaps rename it to `invalid-input-output-args.test`


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:4
+## Missing input file:
+# RUN: not llvm-libtool-darwin -o libOut.a 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NO-INPUT
----------------
Use `%t` or similar, or possibly `/dev/null`, here, even if the tool shouldn't actually write anything, to avoid any remote risk of it writing to the wrong place.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:22
+## Passing in two output files:
+# RUN: not llvm-libtool-darwin %t -o Output1 -o Output2 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DOUBLE-OUTPUT
----------------
Same as above - use a `%t` variant or `/dev/null`.


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


================
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));
+
----------------
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).


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:32
+
+  return EXIT_SUCCESS;
+}
----------------
I don't think you need to explicitly `return EXIT_SUCCESS`. That's the default behaviour of `main` in C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82923