[PATCH] D65541: [llvm-objcopy][MachO] Implement --only-section

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked an inline comment as done.
seiya added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.cpp:15-17
+    LC.Sections.erase(std::remove_if(std::begin(LC.Sections),
+                                     std::end(LC.Sections), ToRemove),
+                      std::end(LC.Sections));
----------------
rupprecht wrote:
> seiya wrote:
> > rupprecht wrote:
> > > SymTable/StrTable are explicitly referenced in MachO/Object.h, so if either of those are matched by the predicate, those tables should be cleared out too. (And there should be test cases to verify this).
> > > 
> > > Also, you might want to use stable_sort to remove sections -- see ELF's implementation.
> > > 
> > > Although specific to --only-section (see `replaceAndRemoveSections`, ELFObjcopy.cpp), StrTable/SymTab should not be implicitly removed, so it's not really observable with just this patch.
> > > SymTable/StrTable are explicitly referenced in MachO/Object.h, so if either of those are matched by the predicate, those tables should be cleared out too. (And there should be test cases to verify this).
> > 
> > Unlike ELF, in MachO, SymTable/StrTable are not sections, but are load commands. Thus, we never remove them by this `removeSections`.
> > 
> > > Also, you might want to use stable_sort to remove sections -- see ELF's implementation.
> > Perhaps you meant `stable_partition`?
> > 
> Yeah, `stable_partition`, good catch.
> 
> In retrospect, I think the use of ELF/removeSections actually may not need to use `stable_partition`, and switching to `remove_if` may be faster. IIUC the difference is that stable_partition guarantees the things past the partition point are also in stable order, whereas remove_if does not (and is faster for not having to). So disregard my original suggestion.
As you say, it seems that we should use `remove_if` instead. According to [[ https://iterator.wordpress.com/2016/01/31/algorithms_0/ | this blog post ]] (lacks the details of the experiment environment though), `remove_if` is faster than `stable_partiton`. I think normally the number of sections are small, so using `remove_if` improve the performance not so much though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65541