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

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:295
 
+    case MachO::LC_RPATH: {
+      StringRef CurrentRPath = getPayloadString(LC);
----------------
nit: are these braces intentional? It's inconsistent with the other cases.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:296
+    case MachO::LC_RPATH: {
+      StringRef CurrentRPath = getPayloadString(LC);
+      OriginalRPaths.insert(CurrentRPath.str());
----------------
I think you have some options available depending on how clever you want to get with sets if you care about minimizing string cloning. Instead of tracking original rpaths, you can track operations on the config rpaths, for example (those strings shouldn't change unexpectedly beneath you, right)?

```
std::set<StringRef> UpdatedRPaths;

case Macho::LC_RPATH:
  StringRef CurrentRPath = ...
  auto FoundArg = ...
  if (FoundArg != end) {
    if (CurrentRPath in UpdatedRPaths) {
      // duplicate scenario
    }
    if (CurrentRPath in RPathToAdd) {
      // Also a dup scenario
    }
    updateLoadCommandPayloadString()
    UpdatedRPaths.insert(FoundArg->first);
  }
...

for (const auto &OldNew : Config.RPathsToUpdate) {
  std::tie(Old, New) = OldNew;
  if (Old not in UpdatedMap) {
    // no load command scenario
  }
  // dup scenario handled above
}

...
std::set<StringRef> AddedRPaths
for (StringRef RPath : Config.RPathToAdd) {
  if (RPath in AddedRPaths) {
    // dup
  }
  AddedRPaths.insert(RPath);
}
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812