[PATCH] D66062: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 4]

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I think some other interesting test cases are a) adding more than one symbol/section, b) adding and then removing a symbol/section, c) removing stuff and then adding stuff, showing that the iteration still works correctly.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:173
+    /// Add's \param Value to the end of the table.
+    NewType &add(NewType &&Value) {
+      NewValues.emplace_back(std::move(Value));
----------------
My instinct is that this should be a `void` return type.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:352
+  /// Add symbol \param Sym.
+  MutableELFSymbol<ELFT> &addSymbol(MutableELFSymbol<ELFT> &&Sym) {
+    return Symbols.add(std::move(Sym));
----------------
I think this should just be a `void` return type.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:357
+  /// Add section \param Sec.
+  MutableELFSection<ELFT> &addSection(MutableELFSection<ELFT> &&Sec) {
+    return Sections.add(std::move(Sec));
----------------
I think this should just be a `void` return type.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:414-419
+MutableELFObject<ELFT>::getSymbol(DataRefImpl Sym) const {
+  const MutableSymbolTable &Table = getWhichTable(Sym);
+  if (const MutableELFSymbol<ELFT> *SymOrNull = Table.getConstIfNew(Sym.d.b))
+    return &SymOrNull->Header;
+  return ELFObjectFile<ELFT>::getSymbol(Sym);
+}
----------------
This seems like an unrelated change?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:523-525
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      4);
----------------
Comment why there's 4 sections already.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:544-546
+  EXPECT_EQ(
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end()),
+      1);
----------------
Comment why there's already a symbol here.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:548
+
+  Elf_Sym_Impl<ELF64LE> SymHeader;
+  SymHeader.st_shndx = ELF::SHN_ABS;
----------------
It's not a header. Symbols are just that - symbols (unlike sections which have a header and actual contents). So this should just be called "Sym" or something like that.


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

https://reviews.llvm.org/D66062