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

[Alex Brachet via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:34
+
+  MutableELFSection(uintptr_t ToCopy, const MutableELFObject<ELFT> *ObjFile)
+      : MutableELFSection(toDataRef(ToCopy), ObjFile) {}
----------------
jhenderson wrote:
> I think a more normal-looking signature would simply be `MutableELFSection(uint8_t *Data, const MutableELFObject<ELF> *ObjFile)`
I'm not sure I understand why it should be a `uint8_t *`, `ELFObjectFile::getSection` takes a `DataRef`


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:44
+template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
+  friend class MutableELFSection<ELFT>;
+  using Base = ELFObjectFile<ELFT>;
----------------
jhenderson wrote:
> Why does this need to exist?
This was referring to the friend declaration before changes.

Because the constructor uses the base method which is protected.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:52
+  Expected<StringRef> getSectionName(DataRefImpl Sec) const override {
+    auto Mapping = Sections[Sec.p];
+    if (Mapping.New) {
----------------
jhenderson wrote:
> Probably shouldn't use auto here.
The typename is really long would you be open to changing it back to auto?


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

https://reviews.llvm.org/D64281