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

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:275
 
+  std::set<std::string> OriginalRPaths;
+  auto FindPair =
----------------
alexshap wrote:
> jhenderson wrote:
> > sameerarora101 wrote:
> > > NOTE: Cannot store `StringRef` in OriginlRPaths (i.e cannot have `set<StringRef> OriginalRPaths`). This is because the StringRef `CurrentRPath` is a reference to the string stored inside LC and thus when we change the payload (via `-rpath` option) `CurrentRPath` is also updated to hold a reference to the new string. Hence I need to store the actual strings inside the set.
> > You probably don't want a `std::set` here. Have you reviewed [[ https://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc | the Programmer's Manual ]] on what set type to use?
> 1. I wanted to mention it on one of the previous diffs but looks like I forgot - it would be better to place all the details related to updating load commands into a separate helper function rather than directly into handleArgs.
>    
> ```
> static Error updateLoadCommands(const CopyConfig &Config, Object &Obj)
> ```
> handleArgs would invoke this function (updateLoadCommands) similarly to removeSections, updateAndRemoveSymbols etc.
> 
> Probably it would be even better first to refactor the code this way (on a separate diff) before moving forward if you don't mind
> (now handleArgs is accumulating the details it should not really be aware of)
> 
> 2. Please, take a look at StringSet, StringMap and their interfaces 
> (in particular, if I am not mistaken they would help you avoid some str() calls below)
>  
> Also this probably suggests that Config.RPathsToUpdate, Config.RPathsToUpdate should really be StringMap-s.
> Also this probably suggests that Config.RPathsToUpdate, Config.RPathsToUpdate should really be StringMap-s.

+1, although does StringMap have deterministic iteration? Maybe SetVector would work as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812