[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/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;
----------------
Would `StringMap` not work instead of `DenseMap`?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:163
+
+  // Get all existing RPaths
+  for (LoadCommand &LC : Obj.LoadCommands) {
----------------
alexshap wrote:
> sameerarora101 wrote:
> > As recommended by @alexshap it is clearer to understand (and the code is cleaner too) if iterate through the LCs twice + use Maps (DenseMaps) down below.
> yeah, thanks!
Nit here and below: you need full stops at the end of your comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:164-167
+  for (LoadCommand &LC : Obj.LoadCommands) {
+    if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH)
+      OriginalRPaths.insert(getPayloadString(LC));
+  }
----------------
I thought you were avoiding iterating twice over the LoadCommands before? Why are this loop and the update loop below separate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812