[PATCH] D82812: [llvm-install-name-tool] Merge rpath with id/change

[Sameer Arora via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:217
+
+  // Add new RPaths.
+  for (StringRef RPath : Config.RPathToAdd) {
----------------
alexshap wrote:
> sameerarora101 wrote:
> > alexshap wrote:
> > > smeenai wrote:
> > > > Nit: do we want to be adding new load commands in a function called `updateLoadCommands`? At least to me that function name seems like it should only be updating existing load commands, since we have a separate `removeLoadCommands` to handle removal. I'll leave it to the more experienced llvm-objcopy reviewers (@alexshap, @jhenderson) to decide if this is okay as-is or if we want a separate `addLoadCommands` function.
> > > so basically the idea was to group together logical pieces of handleArgs (to some reasonable extent).
> > > Besides error-reporting removeLoadCommands is ~10-12 lines of code, so I'd probably inline it into
> > > updateLoadCommands for consistency.
> > @alexshap Instead of inlining the whole `removeLoadCommands` inside `updateLoadCommands` I think it would cleaner if I just call
> > 
> > ```
> >   // Remove LCs.
> >   if (Error E = removeLoadCommands(Config, Obj))
> >     return E;
> > ```
> > from inside `updateLoadCommands`. This can allow for independent development of `removeLoadCommands` in future as well. What do you think? (I have updated the current diff with this change, however, I can update it again in case we want something else)
> I'm not sure that removeLoadCommands is realistically independent from updateLoadCommands, e.g. the order in which you modify the list of load commands appears to be important. Since it's small (~10 lines) it seems preferable to avoid creating this weird asymmetry between removing/adding.
I see. Ok, I have inlined `removeLoadCommands` into `updateLoadCommands` then. Thanks 😊 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812