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

Tue Jul 7 00:33:10 PDT 2020

jhenderson 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
+
----------------
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.

================
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=create-static-lib.test.tmp
+
----------------
Mostly fine, but use `%basename_t` here too.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:34
+
+## Checking that the output file is overwritten:
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-input2.o
----------------
Checking -> Check

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:37
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=OVERWRITE-NAMES --implicit-check-not={{.}}
+# RUN: llvm-nm --print-armap %t.lib | \
----------------
Here and below, use `%basename_t` too, to avoid baking in file names.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:44
+# OVERWRITE-SYMBOLS:      Archive map
+# OVERWRITE-SYMBOLS-NEXT: _symbol2 in create-static-lib.test.tmp-input2.o
+# OVERWRITE-SYMBOLS-EMPTY:
----------------
Ditto.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:50-52
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-NAMES --implicit-check-not={{.}} -DPREFIX=create-static-lib.test.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-SYMBOLS -DPREFIX=create-static-lib.test.tmp
----------------
Ditto.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:7
+
+# MISSING-OPERATION: Library Type:  option: must be specified at least once!
+
----------------
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.

================
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 | \
----------------
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"

================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test:44
+
+# NOT-OBJECT: error: 'invalid-input-output-args.test.tmp.invalid': The file was not recognized as a valid object file
+
----------------
Use `%basename_t` here too.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test:51
+
+# NOT-MACHO: error: 'invalid-input-output-args.test.tmp.elf': format not supported
+
----------------
Use `%basename_t` here too.

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

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

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:100
   InitLLVM X(Argc, Argv);
+  cl::HideUnrelatedOptions({&LibtoolCategory, &ColorCategory});
   cl::ParseCommandLineOptions(Argc, Argv, "llvm-libtool-darwin\n");
----------------
See above re. option category change.

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:34-38
+static cl::opt<Operation> LibraryOperation(
+    cl::desc("Library Type: "),
+    cl::values(
+        clEnumValN(Static, "static",
+                   "Produce a statically linked library from the input files")),
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > I'm not really familiar with the `Operation` type. What does it look like in the help text?
> this is what help text looks like:
> ```
> OVERVIEW: llvm-libtool-darwin
> 
> USAGE: llvm-libtool-darwin [options] <input files>
> 
> OPTIONS:
> 
> Color Options:
> 
>   --color             - Use colors in output (default=autodetect)
> 
> Generic Options:
> 
>   --help              - Display available options (--help-hidden for more)
>   --help-list         - Display list of available options (--help-list-hidden for more)
>   --version           - Display the version of this program
> 
> llvm-libtool-darwin options:
> 
>   -o                  - Alias for -output
>   --output=<filename> - Specify output filename
>   Library Type: 
>       --static           - Produce a statically linked library from the input files
> ```
> 
> I created an `enum Operation` here so that in future we can add support for `dynamic` operation easily. I can very well make the `-static` option a boolean flag as well. What do you think?
Your current approach seems fine. I was just making sure it didn't do anything too weird.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83002