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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:132
+static Error updateLoadCommands(const CopyConfig &Config, Object &Obj) {
+
+  // Remove RPaths.
----------------
Nit: remove blank line at start of function.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:217
+
+  // Add new RPaths.
+  for (StringRef RPath : Config.RPathToAdd) {
----------------
sameerarora101 wrote:
> 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 😊 
There are two options. Either a) rename `updateLoadCommands` to something more generic (e.g. `processLoadCommands`, in which case I'd ensure all load command processing is done in that function), or b) think of it purely in conceptual terms where the load commands in the function name refers to the set of load commands, rather than each individual load command, if that makes sense. Thus you update that set by adding/removing elements, and also changing the existing elements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812