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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/MergedOutputSection.h:19
+
+class MergedOutputSection : public OutputSection {
+public:
----------------
smeenai wrote:
> Ktwu wrote:
> > Ktwu wrote:
> > > int3 wrote:
> > > > 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
> > > Sure.
> > Er, actually, I like MergedOutputSection because it conveys more information about what kind of OutputSection it is. Given how many of the other classes used don't use -Base in their name -- InputSections, OutputSegments -- it feels clunky to have an OutputSectionBase.
> > 
> > @int3 do you feel strongly about renaming this? 
> Sure, I'm good with the parent class being named `OutputSection` and having specific subclasses representing the different types of output sections. I'm not the biggest fan of the `MergedOutputSection` name because to me it suggests output sections being merged together rather than input sections being merged into a single output section, but I can't think of anything better either, so I'm good with it if we can't come up with a better name.
> 
> Naming is hard :D
I prefer the rename because it more closely parallels what lld-ELF has. Moreover ELF's MergeInputSection is quite different, so it's almost like a false parallel...

Re conveying information, my mental model has been that OutputSections are by default mergeable, and only the special SyntheticSections aren't, so I don't feel that there's a need to explicitly call out the mergeability.

`-Base` being a clunky, non-functionally-descriptive suffix is a fair point though...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893