[PATCH] D66063: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 5]

[Alex Brachet via Phabricator via llvm-commits]

abrachet added a comment.

In D66063#1639824 <https://reviews.llvm.org/D66063#1639824>, @rupprecht wrote:

> In D66063#1636993 <https://reviews.llvm.org/D66063#1636993>, @jhenderson wrote:
>
> > 2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.
>
>
> This approach (not private inheritance, but making ELFObjectFile a private member + exposing specific accessors) seems to be the least fragile approach, but also the most verbose one, e.g. you may have to implement/override all the methods of EF that are used in subclasses.


I have been thinking about this a lot recently. I do really like that `MutableELFObject` inherits from `ELFObjectFile` I think it reuses the most amount of code possible while still doing what we want. I think it isn't worth it to use `MutableELFObject` as a private member that it would be too verbose. Do you think it is sufficiently less fragile to warrant making the larger change? Better to be sure now when the code is much easier to change with all patches still open :)

I did have a question about private inheritance and what is safe to do.  I think the most useful methods for llvm-objcopy are actually in the `SectionRef` `SymbolRef` and `SegmentRef` types so really the only methods I can see that need to be publicly exposed are the iterator returning ones. The problem is (not for `SegmentRef` because I can change that) those keep a pointer to an `ObjectFile`. From what I have tested, the virtual table is still constructed correctly and we can `reinterpret_cast<ObjectFile*>(this)` when creating `Segment/SymbolRef`'s and the proper overrides will be called. Is this safe? If not then I guess there would be no difference in how much boilerplate we would need to add between using inheritance and having an ELFObjectFile as a member.

Lastly, how should I go about doing this? Should I switch to private inheritance in this patch or in Part 1 D64281 <https://reviews.llvm.org/D64281>?


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

https://reviews.llvm.org/D66063