[PATCH] D82613: [llvm-install-name-tool] Add -change option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-change.test:67-68
+# RUN: cp %t %t1
+# RUN: llvm-install-name-tool -change /usr/JOJO/LOAD /usr/dylib/LOAD \
+# RUN:                        -change /usr/sh/WEAK /usr/dylib/WEAK %t
+# RUN: cmp %t %t1
----------------
To avoid any possible confusion, I'd recommend changing the output paths here to not match those in the input. The fact that they match suggests that's important otherwise.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:280-283
       if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_ID_DYLIB) {
         updateLoadCommandPayload<MachO::dylib_command>(LC, Id);
         break;
       }
----------------
I think it might make sense to move this logic inside the loop below - it probably doesn't make sense to loop over the load commands multiple times if you specify multiple options. The same might apply in relation the rpath stuff too.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82613