[PATCH] D64281: [Object] Create MutableObject class for doing Object Mutations [Part 1]

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: grimar.
jhenderson added reviewers: Bigcheese, jakehehrlich.
jhenderson added a comment.
Herald added a subscriber: dexonsmith.

Don't forget to update the review summary, following your changes to this patch. It still refers to "UpdatableList" which is obviously out of date. Also don't forget to use the "Update related revisions" option to show the dependency this change has on other reviews.

I've also added more reviewers/subscribers for visibility. They might have some more thoughts on this topic.



================
Comment at: llvm/include/llvm/Object/MutableObject.h:41
+// Custom cloner for MutableRange because MutableXXXBase classes
+// are non trivially copyable.
+template <typename T> struct MutableEntryCloner {
----------------
non -> not


================
Comment at: llvm/include/llvm/Object/MutableObject.h:46
+
+// MutableRange doesn't take ownership of the underlying container
+// this is a small wrapper that does.
----------------
Shouldn't the underlying container be the base ObjectFile class? I don't understand the need for this class.

`MutableRange` should own anything that it instantiates due to modifications, just like the underlying container owns its members.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:68
+
+  MutableObject(const ObjectFile &ObjFile,
+                SectionList::VecType OriginalSections,
----------------
Why can't the MutableObject just query the ObjectFile for its sections etc? Note that ObjectFile provides section_begin and section_end iterators.

Also, I'd be very wary of adding Symbols as a global list. It's technically valid for an ELF to have multiple SHT_SYMTAB instances, although in practice only one of these will be considered by tools as "the symbol table" in the ELF. It probably makes more sense for these to be owned by a symbol table class, and then for the MutableObject to have a reference to that class, which it manipulates.

Finally, Segments are not a type in the ObjectFile class, because they aren't a universal concept (ELF and Mach-O have them, but not COFF for example, I believe). I'm okay with this being specific to ELF for now, but it needs to be designed such that a common base class can be added later to handle the other formats (similar to how there is ObjectFile, ELFObjectFile etc). Perhaps you should just rename this class to MutableELFObject now, to avoid confusion.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:74-75
+public:
+  static Expected<std::unique_ptr<MutableObject>>
+  createMutableObject(const ObjectFile &ObjFile);
+
----------------
I don't understand why you need this. Why can't you have a public constructor?


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

https://reviews.llvm.org/D64281