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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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));
----------------
abrachet wrote:
> jhenderson wrote:
> > My instinct is that this should be a `void` return type.
> Hmm, I'm not sure about this and the other comments. I think it is more convenient this way, but also I wonder when you would add something and then immediately change something. The cost of this return is very small though I would say.
> 
> I don't  have a strong opinion either way.
My thinking is that `add` is akin to vector's `push_back` method, which doesn't return anything.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:545
+
+  // yaml2obj puts in .symtab, .strtab and .shstrtab sections by default.
+  EXPECT_EQ(
----------------
"yaml2obj creates the .symtab ..."


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:567
+
+  // yaml2obj puts in the index 0 symbol in the symbol table even when no
+  // other symbols exist.
----------------
"... creates the index 0 symbol even when ..."


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:613
+  Elf_Sym_Impl<ELF64LE> Sym;
+  for (int _ : seq(0, 10000)) {
+    (void)_;
----------------
`for (size_t I = 0; I < 10000; ++I)` would seem more natural.

I'd also reduce it to 1000 or 100 for speed.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:624
+// Add and then remove a symbol.
+TEST(MutableELFObject, AddRemove) {
+  SmallString<0> Storage;
----------------
AddRemoveSymbol


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:659
+// Add and then remove sections.
+TEST(MutableELFObject, AddThenRemove) {
+  SmallString<0> Storage;
----------------
AddRemoveSection


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:688-705
+  Elf_Shdr_Impl<ELF64LE> Header;
+  Header.sh_size = 2;
+  MutableObject.addSection({Header, "", OwningArrayRef<uint8_t>(0)});
+  EXPECT_EQ(Distance(), 5);
+  EXPECT_EQ(IterAtIndex(4)->getSize(), 2u);
+  MutableObject.removeSection(*(IterAtIndex(1)));
+  EXPECT_EQ(Distance(), 4);
----------------
Could you add some new lines in this block between the groups of setup followed by checks, to make it easier to see where the interesting tests actually are, please.


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

https://reviews.llvm.org/D66062