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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added a comment.

I've added a few comments but other than that this version looks  good to me



================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:161
+static Error updateLoadCommands(const CopyConfig &Config, Object &Obj) {
+  DenseSet<StringRef> OriginalRPaths;
+
----------------
nit: what about renaming OriginalRPaths -> RPaths ?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:163
+
+  // Get all existing RPaths
+  for (LoadCommand &LC : Obj.LoadCommands) {
----------------
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!


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:196
+    case MachO::LC_RPATH: {
+      StringRef CurrentRPath = getPayloadString(LC);
+      StringRef FoundArg = Config.RPathsToUpdate.lookup(CurrentRPath);
----------------
nit: FoundArg doesn't seem to be a very descriptive name
```
StringRef RPath = getPayloadString(LC);
StringRef NewRPath = Config.RPathsToUpdate.lookup(RPath);
...
```


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:207
+    case MachO::LC_LOAD_WEAK_DYLIB:
+      StringRef CurrentInstallName = getPayloadString(LC);
+      StringRef FoundArg =
----------------
```
StringRef InstallName = getPayloadString(LC);
NewInstallName = Config.InstallNamesToUpdate.lookup(InstallName)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812