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

Mon Jul 6 15:27:29 PDT 2020

smeenai added inline comments.

================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:65-66
   // specified in -delete_rpath.
   for (StringRef RPath : Config.RPathsToRemove) {
     if (RPathsToRemove.count(RPath))
       return createStringError(errc::invalid_argument,
----------------
sameerarora101 wrote:
> smeenai wrote:
> > Not related to this diff, but something I noticed: given that `RPathsToRemove` starts out as a copy of `Config.RPathsToRemove` and you're just removing elements from it, can't you just error out for any remaining entries in `RPathsToRemove` instead?
> So the idea here was to raise an error for the **first** specified RPath that doesn't exist. For that purpose, we iterate through `Config.RPathsToRemove` and raise an error for the **first** RPath not deleted. 
Got it. RPathsToRemove is a DenseSet though, and I don't believe that guarantees that iteration order will be the same as insertion order (or even that iteration order will be deterministic); you'd need a [SetVector](https://llvm.org/docs/ProgrammersManual.html#llvm-adt-setvector-h) for that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812