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

[Alex Brachet via Phabricator via llvm-commits]

abrachet added a comment.

> I was discussing this issue with a colleague yesterday, to see if there's a clear path forward. He suggested that it might have made more sense to just modify the ELFObjectFile interface directly, rather than adding the MutableELFObject class.

So remove `MutableELFObject` and put it in `ELFObjectFile`? This might let `ELFObjectFile` no longer depend on `ELFFile` but I'm not entirely sure on that. It's also worth noting that as @rupprecht has pointed out many methods are now linear not constant so without doing some changes (could even be a boolean to track whether any modifications have been made) it probably shouldn't be used yet.

> You could also mess about with more objects to hide away the dangerous methods by doing this.

Is this in reference to the earlier point about just modifying `ELFObjectFile`?

> I believe the virtual table will still exist, if you reinterpret_cast to the base class, and so it's safe enough to work with the base class this way, but it feels a bit wrong to be doing so, as it circumvents the whole point of why we are doing the inheritance this way at all.

I think it isn't that bad to cast `this` to a base class if we know the methods that will be called in that context will always be safe to call. It is much more controlled this way but still bugs can happen if the base class methods get change. I don't feel too strongly.

> One advantage of private inheritance over a private member, I believe, is that you should be able to use the using directive to pull base-class functions into the public interface of the sub-class:

Oh that's very useful I didn't know about that.


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

https://reviews.llvm.org/D66063