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

Thu Jul 2 02:38:29 PDT 2020

jhenderson accepted this revision.
jhenderson added a comment.

LGTM.

================
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;
----------------
sameerarora101 wrote:
> 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?
Makes sense. I haven't looked at the data structures themselves.

FWIW, I don't think taking ownership is necessarily an issue, since there aren't going to be significant numbers of options so the string copying isn't going to be significant.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812