[PATCH] D81887: [lld-macho] Refactor segment/section creation, sorting, and merging

[Jez Ng via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/OutputSegment.h:20
 
-constexpr const char *pageZero = "__PAGEZERO";
-constexpr const char *text = "__TEXT";
-constexpr const char *data = "__DATA";
-constexpr const char *linkEdit = "__LINKEDIT";
-constexpr const char *dataConst = "__DATA_CONST";
+constexpr const char pageZero[] = "__PAGEZERO";
+constexpr const char text[] = "__TEXT";
----------------
for future reference: the change from a pointer to an array was required so that these constants could be used in `StringSwitch`.


================
Comment at: lld/MachO/Writer.cpp:337
 
+static int segmentOrder(OutputSegment *seg) {
+  return StringSwitch<int>(seg->name)
----------------
Ktwu wrote:
> Why move the ordering functions back into the writer?
I thought it was nice for all the sorting code to live together, so I brought the ordering functions next to `sortSegmentsAndSections`. Also while looking up the ordering functions I always forgot if they lived in `OutputSegment.cpp` or `OutputSection.cpp`...

I guess another way to look at it is an OO vs functional style of code organization -- I prefer the latter,  especially since lld is mostly performing complex operations on relatively simple objects


================
Comment at: lld/MachO/Writer.cpp:355
+    return StringSwitch<int>(osec->name)
+        .Case(section_names::binding, std::numeric_limits<int>::min())
+        .Case(section_names::export_, std::numeric_limits<int>::min() + 1)
----------------
Ktwu wrote:
> Why use std::numeric_limits instead of constants like -3, -2, -1, 0? Magic numbers suck but std::numeric_limits<int>::min() seems sort of verbose.
> 
> Nit: cache std::numeric_limits<int>::min() in a temp variable?
fair enough, I switched to -3, .. 0 instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81887