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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

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.



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:385
 
+  virtual const Elf_Ehdr &getHeader() const { return *EF.getHeader(); }
+
----------------
jhenderson wrote:
> Adding a comment here saying make sure to use this function instead of calling the EF.getHeader() function directly is probably a good idea.
Not sure I agree with changing this to a ref; keeping the type as a pointer would probably make this patch (and any potential down-tree fixes) simpler, even if it can't ever be null. I don't feel strongly though.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:357
+  const Elf_Ehdr &getHeader() const override { return FileHeader; }
+  Elf_Ehdr &getMutableHeader() { return FileHeader; }
 };
----------------
This is logically the same thing as `getHeader`, is there a reason it needs to have a different name?


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

https://reviews.llvm.org/D66063