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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 6 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:73
+  section_iterator section_begin() const override {
+    return section_iterator(SectionRef(toDataRef(0UL), this));
+  }
----------------
jhenderson wrote:
> `unsigned long` isn't the right size to contain a pointer on some systems, e.g. 64-bit Windows. I think I'd prefer you to go back to the old approach of taking uintptr_t.
Compiler was getting upset because the `0` literal is treated as an int so the toDataRef was instatiated with [T = int], `reinterpret_cast` can't cast to a wider type, and `static_cast` can't cast a pointer to non pointer type. Now I am just using a C-style cast. I haven't seen anything in the style guide explicitly disallowing their use but I also haven't seen them too often either. Is it ok to use, or should I do `enable_if` specialization for integral and pointer types? It's on line 23 of this file.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:103
+template <typename ELFT>
+uint64_t MutableELFObject<ELFT>::getSectionAddress(DataRefImpl Sec) const {
+  MappingType Mapping = Sections[Sec.p];
----------------
jakehehrlich wrote:
> Are we overriding these getFoo methods? They all seem really bad. Just make one method that returns a mutable section and let the user get the data they need from there.
It's so sections can be mutated and the ObjectFile exposes those sections and their changes from the iterator returned by ObjectFile::sections(). But of course there is a cost to that, the SectionRef's from this mutable class are 2 levels of indirection away from the sections not 1 like its super class, which I think is where your hesitation is?

I personally think it is a big advantage to use ObjectFile because it already exists, has been widely tested and is an interface many (all?) tools already interact with. Of course no tools do any mutations and then subsequently create an ObjectFile from that though.

I am not opposed to having mutated sections get their own iterator, but I'm also not convinced it would be better. Preserving the order of sections or inserting sections is very easy this way for example.


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

https://reviews.llvm.org/D64281