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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:19
+// NewType must have a conversion operator to OrigType.
+template <typename OrigType, typename NewType> class MutableTable {
+  struct MappingType {
----------------
Perhaps this should be nested inside MutableELFObject for now, since it is effectively an implementation detail? Obviously, it would move to MutableObject if that class ever exists.


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


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:61
+  template <typename... Args>
+  NewType *makeMutable(uint64_t Index, Args &&... Arguments) {
+    assert(Index < Mappings.size() && "Out of bounds");
----------------
How about having this return a reference, rather than a pointer? That would seem to be a more natural fit.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:70
+
+  NewType *getIfNew(uint64_t Index) {
+    assert(Index < Mappings.size() && "Out of bounds");
----------------
What is this function useful for?

Returning an Optional would be a nicer interface than returning a nullptr, IMO.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:91
+
+static inline DataRefImpl toDataRef(uintptr_t Ptr) {
+  DataRefImpl Ref;
----------------
Does this function need to exist as a free function, or can it be a private method of MutableELFObject?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:121
+template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
+  friend struct MutableELFSection<ELFT>;
+  using Elf_Shdr = Elf_Shdr_Impl<ELFT>;
----------------
Do you need this to be a friend, or can you just pass in another argument or two (if needed) into the constructor of MutableELFSection?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:127
+
+protected:
+  const Elf_Ehdr &getHeader() const {
----------------
protected? Does this class have any subclasses?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:143
+public:
+  MutableELFObject(ELFObjectFile<ELFT> &B)
+      : ELFObjectFile<ELFT>(std::move(B)),
----------------
Use `explicit` here. Should this be an r-value reference? What do you think?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:160-161
+    Expected<StringRef> Name = getSectionName(Sec->getRawDataRefImpl());
+    if (!Name)
+      return nullptr;
+    return Sections.makeMutable(Sec->getRawDataRefImpl().p, Header, *Name,
----------------
You're going to need to do something with the Error inside Expected here. This function should probably return an Expected too, so that you can pass that error up to a higher level to be handled.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:168
+template <typename ELFT>
+void MutableELFObject<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
+  ++Sec.p;
----------------
What is this function for?


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

https://reviews.llvm.org/D64281