[PATCH] D123279: [lld-macho][nfc] Give non-text ConcatOutputSections order-independent finalization

[Jez Ng via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/ConcatOutputSection.cpp:199-209
+  auto finalizeOne = [&](ConcatInputSection *isec) {
+    isecAddr = alignTo(isecAddr, isec->align);
+    isecFileOff = alignTo(isecFileOff, isec->align);
+    isec->outSecOff = isecAddr;
+    isec->isFinal = true;
+    isecAddr += isec->getSize();
+    isecFileOff += isec->getFileSize();
----------------
int3 wrote:
> oontvoo wrote:
> > Why is the lambda needed here? (It's only got one caller ...)
> > 
> > Why not make it the body of the for loop?
> because I forgot to clean up my copypasta :p good catch
factoring it out into its own method so that TextOutputSection can share it too


================
Comment at: lld/MachO/ConcatOutputSection.h:50
 
-private:
-  void finalizeFlags(InputSection *input);
+  std::vector<ConcatInputSection *> inputs;
 
----------------
oontvoo wrote:
> perhaps make this a private, too? 
> 
This does get used externally (in `offsetToInputSection` as well as `sortSegmentsAndSections`).


================
Comment at: lld/MachO/ConcatOutputSection.h:65-66
+  explicit TextOutputSection(StringRef name) : ConcatOutputSection(name) {}
+  void finalizeContents() override {}
+  void finalize() override;
+  bool needsThunks() const;
----------------
oontvoo wrote:
> could we have some comments here on finalizae() vs finalizeContents() pls?
`OutputSections::finalize()` already has a comment block that talks about `finalizeContents()`. I'll add a comment here to reference it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123279