[PATCH] D76839: [lld-macho] Extend SyntheticSections to cover all segment load commands

[Jez Ng via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/Writer.cpp:122
 
-    for (auto &p : seg->sections) {
+    for (auto &p : seg->getSections()) {
       StringRef s = p.first;
----------------
ruiu wrote:
> Use a concrete type instead of `auto`.
The types appear in the next two lines... is it really necessary to repeat them here?

I did a quick grep, it seems that lld/ELF tends to use explicit types for pairs, but lld/COFF seems to favor `auto`.


================
Comment at: lld/MachO/Writer.cpp:237
+static void sortByOrder(MutableArrayRef<InputSection *> in,
+                        llvm::function_ref<uint32_t(InputSection *s)> order) {
+  std::vector<std::pair<uint32_t, InputSection *>> v;
----------------
ruiu wrote:
> You are always passing `sectionOrder` function as the second argument, so it looks like you can eliminate the second parameter and always using `sectionOrder`.
as mentioned in the comment below, this is likely sharable with a similar function in lld/ELF... I can factor it out now to make the 2nd parameter actually useful


================
Comment at: lld/MachO/Writer.cpp:259-261
+    return linkEditOffset + 1;
+  else
+    return linkEditOffset;
----------------
ruiu wrote:
> So it looks like the function returns only 4 possible values -- 0, 1, linkEditOffset, or linkEditOffset+1. Can't you use 0, 1, 2, 3 instead?
The idea is to make it easily extensible -- there may be other special sections that we want to order before the __LINKEDIT sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76839