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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

@smeenai's comments aside, it looks pretty good to me :)



================
Comment at: lld/MachO/MergedOutputSection.h:19
+
+class MergedOutputSection : public OutputSection {
+public:
----------------
smeenai wrote:
> Could you add a class comment about what this represents?
> 
> I'd prefer renaming `OutputSection` to `OutputSectionBase` and `MergedOutputSection` to just `OutputSection`, to be more in line with ELF, but I don't feel super strongly about that.
+1 on the renaming


================
Comment at: lld/MachO/OutputSection.h:76-77
+
+  // Sort sections within a common segment, which stores according
+  // to string -> section mappings.
+  bool operator()(const std::pair<StringRef, OutputSection *> a,
----------------
"which stores them in a MapVector of section name -> section" seems clearer. I think it's worth pointing out we have a MapVector since it doesn't make sense to sort any other kind of map


================
Comment at: lld/MachO/OutputSection.h:78-79
+  // to string -> section mappings.
+  bool operator()(const std::pair<StringRef, OutputSection *> a,
+                  const std::pair<StringRef, OutputSection *> b) {
+    return sectionOrder(a.first) < sectionOrder(b.first);
----------------
nit: take by const ref


================
Comment at: lld/MachO/OutputSection.h:83
+
+  int operator<(const OutputSectionComparator &b) {
+    return segmentOrder < b.segmentOrder;
----------------
I think `operator<` typically returns a `bool`


================
Comment at: lld/MachO/SyntheticSections.cpp:30
+SyntheticSection::SyntheticSection(const char *segname, const char *name) {
+  // No need to orphan synthetic sections; hook them up when they're made.
+  this->name = name;
----------------
ultra nit: how about "Synthetic sections always know which segment they belong to, so hook them up when they're made"? "No need to orphan" seems a bit weird because it hints that one might naively want to orphan them but doesn't indicate why


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893