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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:1
+//===- MutableObjectTest.cpp ----------------------------------------------===//
+//
----------------
This test file is testing the MutableELFObject class, so should be named accordingly.


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:37
+
+  ASSERT_TRUE(!!ErrOrObj);
+
----------------
Somebody mentioned ASSERT_THAT_EXPECTED in another review. You should consider using that instead of `!!`


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:38-42
+
+  auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
+
+  ASSERT_TRUE(ELFObjFile);
+
----------------
You can probably remove the blank lines in this region.


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:45
+
+  auto getSectionName = [](section_iterator Iter) -> const char * {
+    StringRef Name;
----------------
No need for the trailing return type here, I believe.


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:51
+
+  auto Iter = MutableObject.section_begin();
+  EXPECT_STREQ(getSectionName(Iter), nullptr);
----------------
You should ASSERT that the number of sections is as expected before doing this next bit.


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:69
+
+TEST(MutableELFObject, ChangeSectionContents) {
+  SmallString<0> Storage;
----------------
Same comments apply to this test case as the other one.


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:101
+  auto *MutSec = MutableObject.getMutableSection(FirstSec);
+  MutSec->updateData(ArrayRef<uint8_t>((const uint8_t *)"0000", 5));
+
----------------
How about a literal array of data, rather than trying to use a string literal i.e something like: ArrayRef<uint8_t>({'0', '0', '0', '0', '\0'})?

Also, create the array/string out-of-line, so that it can be reused in the comparison later.

You should probably also use data of different sizes to the original, i.e. both smaller and larger, and even empty. You should also test that you can update an updated entry safely.


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:115
+
+  // check that getSize properly uses the headers sh_size value.
+  EXPECT_EQ(FirstSec->getSize(), 2UL);
----------------
check -> Check

headers -> header's


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:118
+
+  // Check that the StringRef has size 2 because headers sh_size was changed.
+  EXPECT_EQ(Contents->size(), 2UL);
----------------
StringRef -> Contents

headers -> header's


================
Comment at: llvm/unittests/Object/MutableObjectTest.cpp:120
+  EXPECT_EQ(Contents->size(), 2UL);
+}
----------------
You should have test cases for every single public class method you have implemented in MutableELFObject.


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

https://reviews.llvm.org/D64281