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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Mostly looks good, barring @smeenai's comments.



================
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 | \
----------------
sameerarora101 wrote:
> 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.
Similar in concept, but different behaviour (it's checking the behaviour of the `cl::Required` on the `Library operation` option, which is a different thing to the input and output files).


================
Comment at: llvm/test/tools/llvm-libtool-darwin/missing-library-type.test:3-5
+# RUN:   FileCheck %s --check-prefix=MISSING-OPERATION --strict-whitespace
+
+# MISSING-OPERATION: Library Type:  option: must be specified at least once!
----------------
I don't think it's necessary to add --strict-whitespace here, as the exact formatting of the message isn't really the tool's responsibility. I'd get rid of the double space, as if the message was correct, and the option, so that you don't have to modify this test when you fix the command line message.


================
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))
----------------
You can get rid of the `const` too if you want. `StringRef` itself is immutable, so all this does is stop you re-assigning `Member`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83002