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

Wed Jul 1 07:32:54 PDT 2020

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

================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:181-182
   std::vector<StringRef> RPathToAdd;
-  std::vector<std::pair<StringRef, StringRef>> RPathsToUpdate;
-  std::vector<std::pair<StringRef, StringRef>> InstallNamesToUpdate;
+  DenseMap<StringRef, StringRef> RPathsToUpdate;
+  DenseMap<StringRef, StringRef> InstallNamesToUpdate;
   DenseSet<StringRef> RPathsToRemove;
----------------
jhenderson wrote:
> Would `StringMap` not work instead of `DenseMap`?
I think it would work too. However, if I am not mistaken, while comparing `StringSet` and `DenseSet` in a previous diff, @smeenai  suggested that `StringSet` takes ownership of its strings (and copies them), which would be unnecessary copying (and so `DenseSet` would be better). I thought it would be the same case for `StringMap` vs `DenseMap` and so I implemented with `DenseMap`. What do you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812