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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 2 inline comments as done.
pcc added inline comments.


================
Comment at: lld/ELF/Driver.cpp:101
+  Partitions.clear();
+  Partitions.emplace_back();
+
----------------
grimar wrote:
> nit: I am not sure how much `Partitions = {{}};` is readable,
> so I would replace with ` Partitions = {Partition()};`
> Though IMO any of these lines looks probably a bit better than these two lines.
`Partitions = {Partition()};` works for me.


================
Comment at: lld/ELF/Driver.cpp:1419
+  NewPart.Name = PartName;
+  Sym->Partition = NewPart.getNumber();
+}
----------------
grimar wrote:
> grimar wrote:
> > It seems to me this is an overoptimization for such place.
> > 
> > What do you think about the following?
> > ```
> >   Partition P = {PartName};
> >   Sym->Partition = P.getNumber();
> > ```
> Oops. My version was wrong. But anyways something like:
> 
> ```
>   Partitions.push_back({PartName});
>   Sym->Partition = Partitions.back().getNumber();
> ```
Agreed that optimization isn't a consideration at all here.

I don't think your suggested code will continue to work once we start adding more fields (for synthetic sections etc.) to the `Partition` data structure, unless we add a constructor that takes a name. See: https://github.com/pcc/llvm-project/blob/d40cc2e48b560445c4c741cc72a8c6bab47fb2eb/lld/ELF/SyntheticSections.h#L1040

I think I'd argue that it is sightly clearer to let the object be default constructed here and then initialize the field by name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60353