[PATCH] D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1]

[Alex Brachet via Phabricator via llvm-commits]

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

In D64281#1617212 <https://reviews.llvm.org/D64281#1617212>, @jhenderson wrote:

> Since you're introducing a new class in a public header, you should write doxygen comments for at least the public/protected interface, and quite possibly any private bits too. I think if you are careful with those comments too, it will help explain the design of the classes.
>
> I've not looked at the unit tests today. I'll do that tomorrow. The interface looks a lot cleaner than it did when I last looked at this. Hopefully adding comments will make it much clearer what's going on. If you make MutableTable a sub-class, then my concerns about the leaky implementation (i.e. needing to decide between fetching the section properties from the table or the base version) are significantly lessened.


I'm still working on the comments for the private interface although they are coming. I do have just the one for the one public method not inherited from `ObjectFile`.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:55
+
+  const OrigType &getOriginal(uint64_t Index) const {
+    assert(Index < OriginalValues.size() && "Out of bounds");
----------------
jhenderson wrote:
> For what do you envisage this function being useful?
This was referencing `getOriginal` before.

In symbols for example, the top half of the `DataRef `is the section index of the symbol table. In that case I use `getOriginal` to see its sh_type. It wouldn't make sense to use the updated section list in this case I think.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:70
+
+  NewType *getIfNew(uint64_t Index) {
+    assert(Index < Mappings.size() && "Out of bounds");
----------------
jhenderson wrote:
> What is this function useful for?
> 
> Returning an Optional would be a nicer interface than returning a nullptr, IMO.
This was referencing `getIfNew` previously.

It's for methods which could not use just the OrigType to do their job properly. For example, `getSectionName` returns the `Name` member when the section has been modified because there is no way to describe the change to `ELFObjectFile` through its `sh_name` field.

`Optional` does not work with references so far as I can tell. Is this correct? A copy would be too costly for `MutableELFSections` which have an `OwningArrayRef` and `std::string`.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:143
+public:
+  MutableELFObject(ELFObjectFile<ELFT> &B)
+      : ELFObjectFile<ELFT>(std::move(B)),
----------------
jhenderson wrote:
> Use `explicit` here. Should this be an r-value reference? What do you think?
Sounds fine to me. In the unit tests I move the ELFObjectFile but then continue to use it because as @rupprecht pointed out previously the move constructor is just a copy constructor. Is it safe to do this or should I explicitly copy?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:168
+template <typename ELFT>
+void MutableELFObject<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
+  ++Sec.p;
----------------
jhenderson wrote:
> What is this function for?
It advances the `section_iterator`. `MutableELFObject` uses a different iterator to that of `ELFObjectFile` which I belive would have a `moveSectionNext` that did something like `Sec.p += sizeof(Elf_Shdr)` as those just point directly to the section header.


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

https://reviews.llvm.org/D64281