[PATCH] D64077: [ELF] Assert sizeof(SymbolUnion) <= 80

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

No objections to adding the assert, will be worth adding a bit more context as I can see someone modifying Symbol getting an assert fail and not knowing why the assert is there.



================
Comment at: ELF/Symbols.h:469
                 "SymbolUnion not aligned enough");
+  static_assert(sizeof(SymbolUnion) <= 80, "SymbolUnion too large");
 };
----------------
This assert will be the same for each symbol, slightly better for compile time to put in assertSymbols().

I think it will be worth mentioning where the 80 comes from and why the assert is there? I'm guessing it is a soft limit rather than a hard requirement. I think it will be worth a comment:
```
It is important to keep the size of the SymbolUnion small for performance reasons. 80 bytes is a soft limit based on the size of Defined on a 64-bit system.
```
An alternative, assuming Defined is the reference size:
```
    static_assert(sizeof(T) <= sizeof(Defined), "Classes derived from Symbol must be no bigger than Defined");
```
Obviously doesn't stop Symbol or Defined from getting larger though.

  


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D64077