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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 3 inline comments as done.
sameerarora101 added inline comments.


================
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;
       }
----------------
jhenderson wrote:
> 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?
Yup, I moved the `-id` logic in the same loop as `-change` now. 

However, I don't think we can do the same for rpath stuff because of all the error checking we need to perform. For eg., in case of `-add_rpath`, for each rpath to be added, we need to iterate through all LCs anyway so as to make sure we aren't duplicating rpaths. Similarly, in case of `-delete_rpath` we need to iterate through the whole list to throw an error in case we aren't able to find the rpath to be deleted. 

 But yeah, in case of `-id` and `-change` there is no such error checking that requires checking over all LCs, so I was able to put them in the same loop easily.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82613