[PATCH] D60353: ELF: Add basic partition data structures and behaviours.

Thu Apr 18 02:23:04 PDT 2019

grimar added a comment.

I have no more comments/questions, thanks! (last suggestion/question is inlined)

================
Comment at: lld/ELF/Driver.cpp:1396
+  if (NumPartitions == 254)
+    fatal("may not have more than 254 partitions");
+  ++NumPartitions;
----------------
pcc wrote:
> grimar wrote:
> > pcc wrote:
> > > grimar wrote:
> > > > Should it be `error`?
> > > With `fatal` we can avoid needing to worry about what happens in case of overflow. Since I'd expect this error to be uncommon I don't think we need to provide a great experience here.
> > Ok, but now after `Partitions` became a vector, I guess you can just remove this 254 limitation?
> The `Partition` fields in `Symbol` and `InputSectionBase` are 8 bits wide, so we can't remove the limitation without making the fields wider. I'm not sure if we should do that because I don't think we should devote too much space in these common data structures to this feature without a good reason (e.g. users need >254 partitions).
Ok, thanks for the explanation! Perhaps it worst to add this information to the documentation in D60242 then? It wasn't obvious for me where this limitation technically comes from.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60353