[PATCH] D39406: Merge SymbolBody and Symbol into one class, SymbolBody.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

I think this change makes sense at least from a stylistic perspective. The original idea of the split was to separate symbol properties from symbol name properties, but that created other issues like where to put member functions.

This change will increase the size of a local symbol by two words, so it would probably be worth doing some perf measurements.

Might want to update `docs/NewLLD.rst` as well.



================
Comment at: lld/ELF/Symbols.h:54
 
-  Symbol *symbol();
-  const Symbol *symbol() const {
-    return const_cast<SymbolBody *>(this)->symbol();
-  }
+  // Symbol binding. This is on the Symbol to track changes during resolution.
+  // In particular:
----------------
This comment needs to be updated.


================
Comment at: lld/ELF/Symbols.h:152
   // True if this is a local symbol.
   unsigned IsLocal : 1;
 
----------------
I think this change will allow you to remove this field and replace it with a comparison on binding. Probably should be done in a follow-up though.


================
Comment at: lld/ELF/Symbols.h:411
-void replaceBody(Symbol *S, InputFile *File, ArgT &&... Arg) {
-  static_assert(sizeof(T) <= sizeof(S->Body), "Body too small");
-  static_assert(alignof(T) <= alignof(decltype(S->Body)),
----------------
Why drop the asserts?


================
Comment at: lld/ELF/Symbols.h:415
+  alignas(SharedSymbol) char D[sizeof(SharedSymbol)];
+  alignas(LazyArchive) char E[sizeof(LazyArchive)];
+};
----------------
Also want LazyObject here.


https://reviews.llvm.org/D39406