[PATCH] D79460: [lld-macho] Follow-up to D77893

[Jez Ng via Phabricator via llvm-commits]

int3 marked 2 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/OutputSegment.cpp:43
     OutputSection *os = i.second;
-    count += (os->isHidden() ? 0 : 1);
+    count += (os->isNeeded() && !os->isHidden() ? 1 : 0);
   }
----------------
Ktwu wrote:
> I personally find this more complicated to read :(
> 
> Knowing that hidden state isn't as simple as checking isHidden() -- isNeeded() is also needed -- seemed confusing to me :/
I agree that sprinkling `isNeeded()` checks around isn't pretty, but the flip side is that there's no longer an implicit requirement that every implementation of `OutputSection` must maintain the invariant that `isNeeded() == false => isHidden() == false`.

But see also https://reviews.llvm.org/D77893#2022314 -- I think we could further clean things up by filtering out all the unneeded sections earlier.


================
Comment at: lld/MachO/Writer.cpp:117
 
-    if (!seg->isNeeded())
+    if (seg->getSections().empty())
       return;
----------------
Ktwu wrote:
> My bad, was this an actual bug? Why not hide the empty() check within a function?
It wasn't an actual bug, but the check here was to avoid calling `firstSection()` below on an empty sections vector. I think using `isNeeded()` which also checks if any contained sections are needed obscures this (plus it confused me for a bit as to why we had to check isNeeded() here when we were already checking it before calling `writeTo()`).

I'll add a comment here to make its purpose clear


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79460