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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 7 inline comments as done.
sameerarora101 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
----------------
jhenderson wrote:
> 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>
> ```
really nice, thanks!


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:184-185
 
+  // install-name-tool's id option
+  Optional<StringRef> SharedLibId;
+
----------------
jhenderson wrote:
> 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.
yup, I can try creating a diff for this.


================
Comment at: llvm/tools/llvm-objcopy/InstallNameToolOpts.td:27
 
+def id : Option<["-","--"], "id", KIND_SEPARATE>,
+                 HelpText<"Change dynamic shared library id">;
----------------
jhenderson wrote:
> What is this `KIND_SEPARATE` about here and in the other options?
`KIND_SEPARATE` is used to specify an option which is followed by its value. Something like `option <value>`. I picked this definition from here https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Option/OptParser.td.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82410