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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:84
+      uint64_t Count = 0;
+      std::transform(OriginalValues.begin(), OriginalValues.end(),
+                     std::back_inserter(Mappings), [&Count](const OrigType &) {
----------------
`llvm::transform(OriginalValues, ...)`


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:113
+    NewType &makeMutable(uint64_t Index, Args &&... Arguments) {
+      assert(Index < Mappings.size() && "Out of bounds");
+      if (Mappings[Index].Type == MappingType::New)
----------------
I think such `Out of bounds` assertions are probably not very necessary. They will certainly segfault if the user does have out-of-bounds accesses.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:116
+        return NewValues[Mappings[Index]];
+      NewValues.emplace_back(Arguments...);
+      Mappings[Index] = MappingType(NewValues.size() - 1, MappingType::New);
----------------
```
Mappings[Index] = MappingType(NewValues.size(), MappingType::New);
NewValues.emplace_back(std::forward<Args>(Arguments)...);
```


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:150
+
+  /// moveSectionNext must be overriden because the section_iterators in
+  /// MutableELFObject work differently than in ELFObjectFile. In this class,
----------------
typo: overridden

`the section_iterators in MutableELFObject` -> `MutableELFObject::section_begin` ? (Just name it explicitly)


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:184
+  /// @code{.cpp}
+  /// for (const SectionRef &Sec : MutObj.sections())
+  ///   if (!Sec.getAlignment()) {
----------------
`for (SectionRef Sec : MutObj.sections())`

SectionRef has just 2 words. It is usually passed by value.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:187
+  ///     auto MutSecOrErr = MutObj.getMutableSection(Sec);
+  ///     if (!MutSecOrErr)
+  ///       handleError(MutSecOrErr.takeError());
----------------
```
if (auto MutSecOrErr = MutObj.getMutableSection(Sec))
  then;
else
  handleError(...);
```



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:189
+  ///       handleError(MutSecOrErr.takeError());
+  ///     MutSecOrErr->Header.sh_addrallgin = 1;
+  ///   }
----------------
typo: sh_addralign


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

https://reviews.llvm.org/D64281