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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:801
   const Elf_Shdr *EShdr = getSection(Sec);
-  return !isBerkeleyText(Sec) && EShdr->sh_type != ELF::SHT_NOBITS &&
-         EShdr->sh_flags & ELF::SHF_ALLOC;
+  return !ELFObjectFile<ELFT>::isBerkeleyText(Sec) &&
+          EShdr->sh_type != ELF::SHT_NOBITS &&
----------------
Are you sure you want to be calling the base-class method though? Isn't that going to go wrong if Sec is a modified or added section?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:20
+// taking an Iterable::value_type.
+template <typename T> class MutableRange {
+public:
----------------
I've been thinking about how this class is used by its clients. To me, it doesn't seem great that clients have to know whether a given member is new or not (i.e. whether it is stored in NewValues etc). I think it's okay to provide makeMutable to get a mutable reference to one of the original members, but I also feel like getNew should not be in the public interface. Instead, you should provide a function that can take a key or something to look things up with, possibly a MappingType, and return the corresponding item as a const variable.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:24
+    uintptr_t Ptr;
+    bool New;
+
----------------
New -> IsNew

This is a) easier to read, and b) going to play nicer with the upcoming variable name changes.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:21
+
+// Change a sections name and test that SectionRef::getName() returns the new
+// name.
----------------
sections -> section's


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:140
+// the same between ELFObjectFile and MutableELFObject.
+TEST(MutableELFObject, NoChange) {
+  SmallString<0> Storage;
----------------
You probably also need to show that if makeMutable has been called, calls to the various functions in the interface reference the mutable version. Currently, you only do this for the section name.


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

https://reviews.llvm.org/D64281