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

[Kellie Medlin via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/MergedOutputSection.cpp:20
+void MergedOutputSection::mergeInput(InputSection *input) {
+  if (this->inputs.empty()) {
+    this->align = input->align;
----------------
int3 wrote:
> bikeshed: Do we want to prefix all member accesses with `this->`? lld-ELF seems to use it inconsistently, lld-COFF in just a few places... I have a slight preference towards omitting it, but we should be consistent either way
Good point; if I was rigorous, I think I'd prefer omitting it, too.


================
Comment at: lld/MachO/OutputSection.cpp:25-28
+void OutputSection::mergeInput(InputSection *input) {
+  error("Section " + this->name + " in segment " + parent->name +
+        " cannot be merged");
+}
----------------
int3 wrote:
> can we just define this method on MergedOutputSection?
Since output segments just contain output sections now, they're not aware of whether they contain synthetic sections or mergable sections. I'm not sure how to  cast that away (I guess a dynamic cast at runtime, but I figured this would be cheaper).


================
Comment at: lld/MachO/OutputSection.h:20
+
+class OutputSection {
+public:
----------------
int3 wrote:
> How does this class hierarchy compare to that in the ELF implementation? I know they have `SectionBase`, `InputSectionBase`, and `MergeInputSection`... was planning to dig into it eventually but curious if you have looked
I hadn't looked too closely tbh.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893