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

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu marked 9 inline comments as done.
Ktwu added inline comments.


================
Comment at: lld/MachO/OutputSection.h:41
+  // Some sections may allow coalescing other raw input sections.
+  virtual void mergeInput(InputSection *input);
+
----------------
smeenai wrote:
> Ktwu wrote:
> > smeenai wrote:
> > > Is this needed for anything other than MergedOutputSection?
> > No, it's not needed except for MergedOutputSection. Should I dynamically cast each output section before trying to merge an input section into it instead (I wanted to avoid the runtime hit doing that).
> Yeah, dynamically casting wouldn't be ideal. We could leave this as-is, or make `getOrCreateOutputSection` return a `MergedOutputSection *` instead of an `OutputSection *` (idk if that'd cause other complications).
I believe that would require having an assert that the output class being returned, if already created, is in fact a mergeable section. I think a static_cast would be what we'd want, but I like having the explicit assert here in case something goes wrong instead of the undefined behavior of a static_cast gone wrong.


================
Comment at: lld/MachO/Writer.cpp:328-330
+    getOrCreateOutputSegment(isec->segname)
+        ->getOrCreateOutputSection(isec->name)
+        ->mergeInput(isec);
----------------
smeenai wrote:
> Nit: might be nicer to assign these to variables instead of chaining.
I personally prefer chaining :D


================
Comment at: lld/MachO/Writer.cpp:376
   // dyld requires __LINKEDIT segment to always exist (even if empty).
-  getOrCreateOutputSegment(segment_names::linkEdit);
-  // No more segments can be created after this method runs.
+  auto *linkEditSegment = getOrCreateOutputSegment(segment_names::linkEdit);
+
----------------
smeenai wrote:
> Nit: use an explicit type instead of auto
What's up with `auto`? It's not like it's forbidden from use in the style guide:

https://llvm.org/docs/CodingStandards.html#id27

I'm curious about folks' reasoning behind its usage (or discouragement of).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893