[PATCH] D114737: [lld][Macho] Include dead-stripped symbols in mapfile

[Michael Park via Phabricator via llvm-commits]

mpark added inline comments.


================
Comment at: lld/MachO/MapFile.cpp:45
+
+template <bool Live> class SymbolVector {
+public:
----------------
Maybe just `Symbols`? There's not much `vector` about this class aside from an implementation detail. Specifically, you can't use this like a `vector` at all.


================
Comment at: lld/MachO/MapFile.cpp:60
+
+  const std::vector<Defined *> &getData() { return data; }
+
----------------



================
Comment at: lld/MachO/MapFile.cpp:62
+
+  bool isLive() { return Live; }
+
----------------



================
Comment at: lld/MachO/MapFile.cpp:81-82
+            assert(!d->isLive() || !shouldOmitFromOutput(d->isec));
+            liveSymbols.conditionalInsert(d);
+            deadSymbols.conditionalInsert(d);
           }
----------------
Seems like you could just make the correct decision here rather than having `conditionalInsert` do a conditional check.


================
Comment at: lld/MachO/MapFile.cpp:87
+  deadSymbols.sort();
+  return std::make_pair(liveSymbols, deadSymbols);
 }
----------------
Probably want to move these out.


================
Comment at: lld/MachO/MapFile.cpp:158
+    for (Symbol *sym : symbols.getData()) {
+      if (symbols.isLive()) {
+        os << format("0x%08llX\t[%3u] %s\n", sym->getVA(),
----------------
Seems silly to check the liveness of the container for each element.
It'd be different if you could write C++17 and turn this into an `if constexpr`,
but otherwise I'd say at least pull this out of the loop such that you only
check this once? Yes the loop would be duplicated but 🤷‍♂️


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114737