[PATCH] D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2]

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:74
+  /// The design of MutableTable allows fewer copies to be made than there
+  /// would otherwise need to be, entries with no modifications never get
+  /// copied and the only overhead for those is an index. Entries which get
----------------
be, entries -> be: entries


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:147
+
+    size_t originalSize() const { return OriginalValues.size(); }
   };
----------------
Does this belong to a later patch?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:185
+
+  const MutableSymbolTable &getWhichTable(DataRefImpl Sym) const {
+    return Sections.getOriginal(Sym.d.a).sh_type == ELF::SHT_SYMTAB
----------------
I'd be inclined to just call this `getSymbolTable`.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:241-243
+  Expected<MutableELFSymbol<ELFT> &> getMutableSymbol(symbol_iterator Sym) {
+    return getMutableSymbol(*Sym);
+  }
----------------
This overload seems a bit superfluous to me.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:246
+  /// Returns a mutable reference to the dynamic symbol at the specified index.
+  Expected<MutableELFSymbol<ELFT> &> getDynMutableSymbol(SymbolRef Sym) {
+    Expected<StringRef> Name = getSymbolName(Sym.getRawDataRefImpl());
----------------
Call this `getMutableDynamicSymbol`


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:255-257
+  Expected<MutableELFSymbol<ELFT> &> getMutableDynSymbol(symbol_iterator Sym) {
+    return getMutableDynSymbol(*Sym);
+  }
----------------
This overload seems a bit superfluous to me.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:270
   /// @endcode
-  Expected<MutableELFSection<ELFT> &> getMutableSection(SectionRef Sec) {
-    const Elf_Shdr_Impl<ELFT> &Header = Sections[Sec.getRawDataRefImpl().p];
-    Expected<StringRef> Name = getSectionName(Sec.getRawDataRefImpl());
-    if (!Name)
-      return Name.takeError();
-    return Sections.makeMutable(Sec.getRawDataRefImpl().p, Header, *Name, this);
-  }
+  Expected<MutableELFSection<ELFT> &> getMutableSection(SectionRef Sec);
 
----------------
This change seems to be unrelated?

Avoid refactors of code which don't have anything to do with the current change, and put them in their own change.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:332
+template <typename ELFT>
+uint32_t MutableELFObject<ELFT>::findSectionOfType(uint64_t ShType) const {
+  for (uint32_t I = 0; I < Sections.originalSize(); ++I)
----------------
Why is this looping over only the original sections?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:241
+
+  auto TestSym = symbol_iterator(++MutableObject.symbol_begin());
+  EXPECT_EQ(TestSym->getValue(), 0x1234U);
----------------
Looks to me like you need a checking first of all counting the number of  symbols. Also, why do you need to construct a new `symbol_iterator`, rather than using the output of `++MutableObject.symbol_begin()` directly?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:248
+
+  auto SecondSym = symbol_iterator(++TestSym);
+  EXPECT_EQ(SecondSym->getValue(), 1U);
----------------
FirstSym and SecondSym?

Also, same comment as above.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:256
+
+// Test that there is no change public methods on SymbolRef between
+// MutableELFObjectFile and ELFObjectFile.
----------------
"no change public methods" what are those? :)

I think you need to rewrite this comment.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:286
+  ASSERT_TRUE(ELFObjFile);
+  const ObjectFile &ObjFile = *ELFObjFile;
+
----------------
I think the same comments made to patch 1 apply here too, regarding the std::move-ing of ELFObjFile making this reference invalid.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:295
+
+  auto TestSymbols = [](SymbolRef ObjFile, SymbolRef MutObj) {
+    EXPECT_EQ(ObjFile.getValue(), MutObj.getValue());
----------------
I think less confusing names might be `FromBase` and `FromMutable` or something like that. My first reaction was "why is a file a `SymbolRef`?"


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:323
+
+  for (const SymbolRef &Sym : MutableObject.symbols()) {
+    auto MutSymOrErr = MutableObject.getMutableSymbol(Sym);
----------------
This loop needs a comment explaining its purpose and the `consumeError` call.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:333
+
+// Test basic public methods on dynamic symbols.
+TEST(MutableELFObject, MutateSymbol) {
----------------
Is this comment correct?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:354
+
+  auto TestSym = symbol_iterator(++MutableObject.symbol_begin());
+  Expected<MutableELFSymbol<ELF64LE> &> MutSymOrErr =
----------------
Same comments as earlier.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:360
+  MutSymOrErr->Header.st_value = 5;
+  EXPECT_EQ(MutSymOrErr->Header.st_value, 5u);
+
----------------
This seems a bit redundant...

Same goes for the other changes below. You're basically testing that C++ works...

I think you want to be fetching the symbol again from one or both of the object interfaces (base or sub-class) and showing that the value has been updated.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:372
+// Test basic public methods on dynamic symbols.
+TEST(MutableELFObject, BasicDynSymb) {
+  SmallString<0> Storage;
----------------
Don't abbreviate names unnecessarily: use "BasicDynamicSymbol"


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:392
+
+  auto DynSym = symbol_iterator(++MutableObject.dynamic_symbol_begin());
+  EXPECT_EQ(DynSym->getValue(), 0x1234U);
----------------
Same comments as above.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:398
+  EXPECT_EQ(*NameOrErr, "test");
+}
----------------
My gut reaction was "where are the equivalent dynamic symbol tests for the earlier regular symbol cases?" Perhaps worth a comment to that effect somewhere, perhaps making it clear that the earlier cases test common code.


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

https://reviews.llvm.org/D65367