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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 3 inline comments as done.
abrachet added a comment.

In D66063#1634844 <https://reviews.llvm.org/D66063#1634844>, @jhenderson wrote:

> I'm concerned with the `getHeader()` call changes in `ELFObjectFile`, mostly because again it's introducing a very obvious point of failure. What if somebody added a new piece of functionality to the base class to get another piece of data from the header? There's a good change they'll call `EF.getHeader()`. Say the new function is called `getFoo()`: if another user then decides they want to call `getFoo()` on a `MutableELFObject`, then they'll end up using the unmodified header rather than the modified one, which could cause surprising bugs. I don't have an obvious answer to this, but I'm beginning to wonder if we should be using a different style of inheritance, one that only makes the functions available that we know to be safe from a `MutableELFObject`'s point of view. This might be something like private inheritance. I'm not sure though, as it would mean providing shim functions to re-publicise the "safe" functions we care about. What are your thoughts?


Hmm that's a lot to think about. I don't love private inheritance because I think it would end up being a lot of boiler plate for the methods that are fine either way, I think.

Certainly it's true that `EF.getHeader()` would be gravitated towards for future changes. I think if a `getFoo()` is added and its something that `MutableELFObject` would want, then either way there needs to be a change either to use `getHeader` not `EF.getHeader()` or add `getFoo()` in `MutableELFObject`. Of course there's a difference because the bug couldn't exist if `getFoo()` isn't available.

You would know more about maintaining something like this so I'll go with whatever you (or others) have to say.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:366
+  const Elf_Ehdr &header() const { return FileHeader; }
+  Elf_Ehdr &header() { return FileHeader; }
 };
----------------
jhenderson wrote:
> Rename this to `getHeader()` to ensure sensible function overloading, and have it return a pointer to the header, like the other version.
I have moved to have a separate `getMutableHeader` that returns a mutable reference.


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

https://reviews.llvm.org/D66063