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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You should write unit tests to test that the class actually works.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:73
+  section_iterator section_begin() const override {
+    return section_iterator(SectionRef(toDataRef(0UL), this));
+  }
----------------
`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.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:104-106
+  MappingType Mapping = Sections[Sec.p];
+  DataRefImpl Ref = Mapping.New ? toDataRef(Sections.getNew(Mapping.Ptr))
+                                : toDataRef(Mapping.Ptr);
----------------
This pattern is repeated everywhere. Should it be factored out into a separate function?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:47
+
+  using MappingType =
+      typename MutableRange<MutableELFSection<ELFT>>::MappingType;
----------------
abrachet wrote:
> jhenderson wrote:
> > Do you anticipate this being used in other future functions within this class? If not, move it inside the function, to reduce its effect to where it's actually used.
> Yes all of the getSectionX methods will need this type. Those haven't been implemented in this patch because I didn't want to overload this patch. But calling those methods for an updated section is undefined behavior. I'm not sure if that means it needs to be in this patch or if it makes sense to move it to the part 2.
I'd be inclined the add all the getSectionX/isSectionX methods in this patch. It gives you something tangible to test too, so clearly demonstrates that the class works. Best thing to do is probably work on the principle that users of this class should be able to use this safely. Alternatively, just return "not supported" errors in all the ones you don't want to support for now. We should avoid leaving cases where things will be confusing to anybody who tries to pick this up and use it after the first commit.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:47
+        [&Extract](typename Iter::value_type Value) -> MappingType {
+          auto Extracted = Extract(Value);
+          assert(Extracted <= (UINT64_MAX >> 1) && "returned type too large");
----------------
abrachet wrote:
> jhenderson wrote:
> > Another instance of too much auto.
> I really don't know the return type of Extractors::operator(), though. I could just make it a function_ref returning a uintptr though I suppose.
In this context, since Extracted needs to be a uintptr_t, you could have just done the cast earlier.


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

https://reviews.llvm.org/D64281