[PATCH] D82923: introducing llvm-libtool-darwin

[Sameer Arora via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/tools/llvm-libtool-darwin/help-message.test:5
+# 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
+# RUN: not llvm-libtool-darwin --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
----------------
MaskRay wrote:
> Does cctool libtool use `-long-option` as the canonical spelling?
> 
> For comparison, GNU getopt convention is `--long-option`
Yup, it uses `-long-option`.


================
Comment at: llvm/tools/llvm-libtool-darwin/LLVMBuild.txt:1
+;===- ./tools/llvm-libtool-darwin/LVMBuild.txt --------------------------*- Conf -*--===;
+;
----------------
jhenderson wrote:
> This line doesn't look quite right. It looks too long and might need fixing. Is it normal to include the path here also? That seems a little weird to me.
oh sorry abt that, I forgot to remove `---` from the ends in order to align the line. I'll update this.

But yeah, for every other tool I checked (`ar`,`link`,`lipo`,`lto`,`size`) they have the path.


================
Comment at: llvm/tools/llvm-libtool-darwin/LLVMBuild.txt:20
+parent = Tools
+required_libraries = Object Option Support
----------------
MaskRay wrote:
> Have you tried a `-DBUILD_SHARED_LIBS=on` cmake build and ensured `ninja llvm-libtool-darwin` can build? `-DBUILD_SHARED_LIBS=on` sometimes has stricter dependency requirement. Every direct dependency must be listed to make `-z defs` happy.
yup, I tried just now and it passes. Thanks.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:32-40
+static cl::opt<std::string> OutputFile("output",
+                                       cl::desc("Specify output filename"),
+                                       cl::value_desc("filename"),
+                                       cl::OneOrMore);
+static cl::alias OutputFileShort("o", cl::desc("Alias for -output"),
+                                 cl::aliasopt(OutputFile));
+
----------------
jhenderson wrote:
> If you are going to add the input/output file options, you need test cases showing what is/isn't accepted in terms of number of arguments and option names.
ok added.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:35
+                                       cl::value_desc("filename"),
+                                       cl::OneOrMore);
+static cl::alias OutputFileShort("o", cl::desc("Alias for -output"),
----------------
jhenderson wrote:
> Are you sure you want this as `OneOrMore`?
So I was thinking of allowing user to change the output file by passing in another `-o` option at the end (since we pick the last one here). But I just checked and cctool's libtool doesn't allow this. So, in order to keep the behavior same, i'll change this to `cl::Required`. Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82923