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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline 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));
+  }
----------------
alexshap wrote:
> jhenderson wrote:
> > I thought you were avoiding iterating twice over the LoadCommands before? Why are this loop and the update loop below separate?
> @jhenderson - I think this was my suggestion, imo splitting these steps makes the code easier to reason about.
> From the performance perspective it doesn't matter much, even huge binaries have ~70 load commands.
Also if I'm not mistaken it has enabled @sameerarora101 to move error reporting (lines 171 -180) earlier, before any changes / updates are made. This makes sense to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812