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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D64281#1593246 <https://reviews.llvm.org/D64281#1593246>, @jhenderson wrote:

> 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.


Don't forget this bit.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:12-13
+
+#include "ELFObjectFile.h"
+#include "MutableObject.h"
+#include "llvm/ADT/ArrayRef.h"
----------------
These should be prefixed with "llvm/Object/" (see e.g. ELFObjectFile.h for this sort of example).


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:21
+
+static DataRefImpl toDataRef(uintptr_t Num) {
+  DataRefImpl Ref;
----------------
Num -> Ptr?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:34
+
+  MutableELFSection(uintptr_t ToCopy, const MutableELFObject<ELFT> *ObjFile)
+      : MutableELFSection(toDataRef(ToCopy), ObjFile) {}
----------------
I think a more normal-looking signature would simply be `MutableELFSection(uint8_t *Data, const MutableELFObject<ELF> *ObjFile)`


================
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>;
----------------
Why does this need to exist?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:45
+  friend class MutableELFSection<ELFT>;
+  using Base = ELFObjectFile<ELFT>;
+
----------------
I'm not sure this gives us anything significant. Just use the type directly (it's not like it's used widely).


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:52
+  Expected<StringRef> getSectionName(DataRefImpl Sec) const override {
+    auto Mapping = Sections[Sec.p];
+    if (Mapping.New) {
----------------
Probably shouldn't use auto here.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:54
+    if (Mapping.New) {
+      auto *MutSec = Sections.getNew(Mapping.Ptr);
+      return MutSec->Name;
----------------
How about `NewSec`?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:75
+
+  MutableELFSection<ELFT> *mutateSection(section_iterator Sec) {
+    auto Index = Sec->getRawDataRefImpl().p;
----------------
How about `getMutableSection`?


================
Comment at: llvm/include/llvm/Object/MutableObject.h:13
+#include "ELFObjectFile.h"
+#include "ObjectFile.h"
+
----------------
I'd be surprised if you need this second include.

Also see above re. include paths.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:18
+
+// T is a wrapper type around Iterable::value_type and must have an conversion
+// operator and a constructor taking a Iterable::value_type.
----------------
an conversion -> a conversion


================
Comment at: llvm/include/llvm/Object/MutableObject.h:19
+// T is a wrapper type around Iterable::value_type and must have an conversion
+// operator and a constructor taking a Iterable::value_type.
+template <typename T> class MutableRange {
----------------
taking a -> taking an


================
Comment at: llvm/include/llvm/Object/MutableObject.h:22
+public:
+  // The second variant of the union cannot be T because T might be non
+  // trivally copyable.
----------------
might be non -> might not be

Also, what union?


================
Comment at: llvm/include/llvm/Object/MutableObject.h:34
+  using iterator = typename std::vector<MappingType>::iterator;
+  using const_iterator = iterator;
+  using value_type = MappingType;
----------------
Defining const_iterator to a non-const iterator type seems like a bad idea to me. Does it need to exist?


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

https://reviews.llvm.org/D64281