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

[Roger Kim via Phabricator via llvm-commits]

Roger added inline comments.


================
Comment at: lld/MachO/MapFile.cpp:45
+
+template <bool Live> class SymbolVector {
+public:
----------------
mpark wrote:
> 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.
True.


================
Comment at: lld/MachO/MapFile.cpp:60
+
+  const std::vector<Defined *> &getData() { return data; }
+
----------------
mpark wrote:
> 
I just got rid of that function.


================
Comment at: lld/MachO/MapFile.cpp:62
+
+  bool isLive() { return Live; }
+
----------------
mpark wrote:
> 
I got rid of that function.


================
Comment at: lld/MachO/MapFile.cpp:81-82
+            assert(!d->isLive() || !shouldOmitFromOutput(d->isec));
+            liveSymbols.conditionalInsert(d);
+            deadSymbols.conditionalInsert(d);
           }
----------------
mpark wrote:
> Seems like you could just make the correct decision here rather than having `conditionalInsert` do a conditional check.
True. I was trying out pushing conditionals into data structures but this may be taking it too far, yeah.


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


================
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(),
----------------
mpark wrote:
> 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 🤷‍♂️
I really wanted to make this into an if constexpr. I'll change this to just be two loops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114737