[PATCH] D82410: [llvm-install-name-tool] Add -id option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test:24
+## Check that -id option has no effect if binary is not a shared dylib:
+# RUN: yaml2obj %p/Inputs/x86_64.yaml -o %t
+# RUN: cp %t %t1
----------------
Am I right in thinking that you're just using the x86_64.yaml input because it doesn't have an LC_ID_DYLIB load command?

I have a personal preference to keep inputs local to their usage where possible, as it makes the test self-contained. To achieve that in this case, you can use yaml2obj's --docnum option to allow multiple YAML docs in the same test file. I'd do something like:

```
# RUN: yaml2obj %s --docnum=1 -o %t
<tests and checks using the first doc>

--- !mach-o
<yaml doc 1>

# RUN: yaml2obj %s --docnum=2 -o %t
<tests and checks using the second doc>

--- !mach-o
<yaml doc 2>
```


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test:29-33
+## Missing id argument:
+# RUN: not llvm-install-name-tool %t -id 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MISSING
+
+# MISSING: missing argument to -id option
----------------
This is somewhat similar to the empty argument case. I'd move it to be adjacent in the test file.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:184-185
 
+  // install-name-tool's id option
+  Optional<StringRef> SharedLibId;
+
----------------
It's really that important, but I wonder if it's time to refactor out the Mach-O specific options into a MachOCopyConfig, a bit like the existing ELF config? Just something to think about, and not part of this patch.


================
Comment at: llvm/tools/llvm-objcopy/InstallNameToolOpts.td:27
 
+def id : Option<["-","--"], "id", KIND_SEPARATE>,
+                 HelpText<"Change dynamic shared library id">;
----------------
What is this `KIND_SEPARATE` about here and in the other options?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82410