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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked an inline comment 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;
       }
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > sameerarora101 wrote:
> > > 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.
> > I think it might be possible with some extra refactoring, but I'd recommend deferring that to a separate patch. What I'm thinking in semi-pseudo code terms is roughly:
> > 
> > ```
> > std::unordered_set<StringRef> RPaths; // Pick your container of choice here. Also useful for the add rpath loop.
> > std::unordered_set<StirngRef> OriginalRPaths;
> > ...
> > case MachO::LC_RPATH:
> >   StringRef Current = getLoadCommandPayloadString(LC);
> >   auto FoundArg = find_if(Config.RPathsToUpdate, IsFirstMatching);
> >   OriginalRPaths.insert(Current);
> >   if (!RPaths.insert(Current).second) // If already in RPaths...
> >     // report duplicate rpath error.
> >   if (FoundArg != Config.RPathsToUpdate.end())
> >     updateLoadCommandPayloadString<MachO::rpath_command>(Current, FoundArg->second);
> > ...
> > // After loop.
> > for (const auto &RPair : Config.RPathsToUpdate) {
> >   if (OriginalRPaths.count(RPair.first) == 0)
> >     // Did not find rpath error.
> > }
> > ```
> I see now. Yup, this would be a nice change. I'll push a separate diff for this change. Thanks a lot 😊
Ok, merged `-rpath` option inside the same loop as `-id` and `-change` here https://reviews.llvm.org/D82812. I refactored common logic too. In fact, it even simplified `-add_rpath` a little. Thanks a lot 😄 and please review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82613