[PATCH] D60297: [COFF] Pack Name in Symbol as is done in ELF

Fri Apr 5 15:10:26 PDT 2019

rnk marked an inline comment as done.
rnk added inline comments.

================
Comment at: lld/COFF/Symbols.h:111
+  // Symbol name length. Assume symbol lengths fit in a 32-bit integer.
+  unsigned NameLen;
+
----------------
thakis wrote:
> aganea wrote:
> > aganea wrote:
> > > ruiu wrote:
> > > > ruiu wrote:
> > > > > I'd use uint32_t to match the comment.
> > > > We have the same hack for ELF, and we name these variables NameSize and NameData.
> > > You could save more by packing this with the bitfield above. ‘SymbolKind’ doesn’t need more than 4 bits, and that would free 21 bits for ‘NameLen’. In practical terms, do we really expect symbol lengths > 2^21 chars? Worst case, there could be a slower/higher mem. codepath for off-the-limits case?
> > Forget what I said, it’ll be aligned on 64-bits because of the pointer that follows.
> If uint32_t happens to be a long, will this prevent packing with the bitfields in the ms abi? maybe unsigned is better for that reason?
Yes, and I think that's part of why I used unsigned in the first place. However, right now it's not a bitfield so it won't matter. If we do ever shrink NameSize even further with bitfields, then yes, we'd want to update them all to some common type.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60297