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

Tue Jun 30 02:08:17 PDT 2020

alexshap added inline comments.

================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:275

+  std::set<std::string> OriginalRPaths;
+  auto FindPair =
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812