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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with one suggestion in the test.



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test:68-72
+LoadCommands:
+  - cmd:             LC_RPATH
+    cmdsize:         32
+    path:            12
+    PayloadString:   '@executable_a/.'
----------------
Can you get rid of the `LoadCommands` here completely?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:184-185
 
+  // install-name-tool's id option
+  Optional<StringRef> SharedLibId;
+
----------------
sameerarora101 wrote:
> 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.
Just to be clear, in case it wasn't obvious, I meant "It's NOT really that important..." - see if it's worthwhile and why we do it for the ELF case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82410