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

[Sameer Arora via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:11
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not={{.}} -DPREFIX=create-static-lib.test.tmp
+
----------------
jhenderson wrote:
> It's not widely used, but there is `%basename_t` which substitutes for just the file name part of `%t`. This allows the test to not make assumptions about how `%t` expands, and also keeps the check independent of the test name. I'd recommend trying that.
thanks, I now have `-DPREFIX=%basename_t.tmp` as `%basename_t` substitutes for the last path component of %t but without the .tmp extension


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:7
+
+# MISSING-OPERATION: Library Type:  option: must be specified at least once!
+
----------------
jhenderson wrote:
> sameerarora101 wrote:
> > jhenderson wrote:
> > > Does the double space match the actual error message?
> > Yes, the actual error msg also has the double space:
> > ```
> > Library Type:  option: must be specified at least once!
> > ```
> Okay, bonus marks for fixing it in another patch if you want. Or file a bug against the CommandLine code.
😂I can fix it in another patch.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test:27
+
+## Missing -static option:
+# RUN: not llvm-libtool-darwin -o %t.lib %t.input 2>&1 | \
----------------
jhenderson wrote:
> Maybe make this more generic, e.g. "Missing library type option:"
> 
> I don't really know where to put this test. It might want to be its own test case entirely, e.g. "missing-library-type.test"
Isn't it similar to `## Missing output file`? (That's why I thought it would go here). But I have placed it in `missing-library-type.test` for now.


================
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"),
----------------
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


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:85
+  std::vector<NewArchiveMember> NewMembers;
+  for (const StringRef &Member : InputFiles)
+    if (Error E = addMember(NewMembers, Member))
----------------
jhenderson wrote:
> sameerarora101 wrote:
> > @jhenderson  Should I replace the type of `Member` back to `auto`. clang-tidy raises a warning with `StringRef`?
> Well typically you don't want to use a `const &` for `StringRef` because it is already a light-weight construct (much the same as you wouldn't use `const &` to a pointer or primitive type). That is probably the cause of the problem here.
ok thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83002