[PATCH] D83002: [llvm-libtool-darwin] Add support for -static option

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:18
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS -DPREFIX=%basename_t.tmp
+
----------------
Might wanna add `--match-full-lines` here, to ensure there's no unexpected symbol name prefix or member name suffix.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:32
+# FORMAT-NEXT: [[PREFIX]]-input2.o
+# FORMAT_NOT:  {{.}}
+
----------------
You have an underscore instead of a dash :)

Is the purpose to ensure that there's no other members? I assume a -EMPTY would work for that. We should also check for the "Archive : " header, to ensure there's no members before the table of contents member.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:47
+
+## Duplicate a binary:
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o %t-input2.o %t-input1.o
----------------
Might be worth noting that cctools libtool warns in this case, and we don't implement that warning yet.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/hide-unrelated-options.test:1
+## This test checks that unrelated options are hidden in help text.
+
----------------
This seems unrelated to this diff; perhaps it should be in the previous one?


================
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.
 
----------------
Can you add `-static` to all of the tests in this file, so that the only invalid part of the command line is the aspect being tested?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:28
+                                       cl::value_desc("filename"), cl::Required,
+                                       cl::cat(LibtoolCategory));
 static cl::alias OutputFileShort("o", cl::desc("Alias for -output"),
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > Adding the categories sounds like a different change to me? You might want to include it alongside a test case to show that unrelated options aren't incldued.
> yup, I added `OptionCategory` in order to prevent the general option `--safepoint-ir-verifier-print-only` from showing up in the help text. Added a test case for it as well. Thanks
Yup, this should either go in the previous diff or be its own diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83002