[PATCH] D77893: [lld] Merge Mach-O input sections

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/OutputSegment.h:51
 
-  const llvm::MapVector<StringRef, std::vector<InputSection *>> &
-  getSections() const {
-    return sections;
-  }
+  const SectionMap &getSections() const { return sections; }
+  size_t numNonHiddenSections() const;
----------------
Ktwu wrote:
> int3 wrote:
> > if `sections` isn't private any more, it doesn't need an accessor
> > 
> > edit: I see that it's non-private only because we need to sort it. How about making the sorting a method on this class?
> > 
> > Related thought... I see that MapVector has a `takeVector` method that clears out the map and returns the underlying vector. Maybe we could do that -- have a `getSortedSections` method that returns an empty vector until we actually sort things. That would mean that the comparator can work on an actual vector & take single elements instead of `std::pair`s. Just my 2c, might be overcomplicating things here
> Yes to section sorting living here, nay to the `takeVector` idea (at least in this diff).
I was looking at the implementation of `MapVector` today and I realized that the sort only operates on the vector and not the map, so the container exhibits some questionable behavior after sorting :D

   MapVector<int, int> mv;
   mv[2] = 98;
   mv[1] = 99;
   std::sort(mv.begin(), mv.end());
   for (int i = 1; i <= 2; ++i)
     fprintf(stderr, "%d %d\n", i, mv[i]);

This prints

  1 98
  2 99

So I think we should really use `takeVector` before sorting. Two more refactoring ideas to tack on to that:

1) We could filter out the unneeded OutputSections as this stage too, so we don't have to worry about checking isNeeded() afterward.
2) Maybe we could consider not creating the OutputSegments till after the sorting/filtering has been done, so we don't have the OutputSegments in a state where some operations aren't valid. But that would probably mean an additional `outputSections` global, so there's some tradeoff there. Up to you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893