[PATCH] D82051: [llvm-install-name-tool] Add -rpath option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:25
 
+static StringRef getLoadCommandPayload(const LoadCommand &LC) {
+  assert((LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH ||
----------------
sameerarora101 wrote:
> I haven't added an assert for LC_REEXPORT_DYLIB , LC_LOAD_UPWARD_DYLIB or LC_LAZY_LOAD_DYLIB as llvm-objcopy doesn't support them yet. I can add a TODO comment here if needed?
TODO comments for missing functionality is usually reasonable, though I wonder whether we should just have an unsupported error somewhere for such cases.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:134-142
+  assert((LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH ||
+          LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_ID_DYLIB ||
+          LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_LOAD_DYLIB ||
+          LC.MachOLoadCommand.load_command_data.cmd ==
+              MachO::LC_LOAD_WEAK_DYLIB) &&
+         "LC_RPATH or \
+          LC_ID_DYLIB or \
----------------
Since this assert is common between the two functions, it might be worth pulling it into a simple helper. Something like "isLoadCommandWithPayload" and just call it instead:

```
assert(isLoadCommandWithPayload(LC && "unsupported load command encountered");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82051