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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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?



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:365
+
+  const Elf_Ehdr &header() const { return FileHeader; }
+  Elf_Ehdr &header() { return FileHeader; }
----------------
Why do you need both this and `getHeader()`?


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


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

https://reviews.llvm.org/D66063