[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/unittests/Object/MutableELFObjectTest.cpp:241
+
+  auto TestSym = symbol_iterator(++MutableObject.symbol_begin());
+  EXPECT_EQ(TestSym->getValue(), 0x1234U);
----------------
abrachet wrote:
> jhenderson wrote:
> > 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?
> `SymbolicFile::symbol_begin()` returns a `basic_symbol_iterator` pointing to `BasicSymbolRef`s whose only meaningful method for testing is `getFlags()`, but `symbol_iterator` points to `SymbolRef`s which has `getValue` and `getName`.
Okay. Please write this as:

`symbol_iterator FirstSym(++MutableObject.symbol_begin());`


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:255-257
+  EXPECT_EQ(
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end()),
+      3);
----------------
I'm not sure you need this here?


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:260
+
+// Test that there is no change between SymbolRef's public methods between
+// MutableELFObjectFile and ELFObjectFile.
----------------
Perhaps "in SymbolRef's public methods beteween..." to avoid using "between" twice.


================
Comment at: llvm/unittests/Object/MutableELFObjectTest.cpp:323
+
+  for (const auto &Tuple : zip(ELFObjFile->symbols(), MutableObject.symbols()))
+    TestSymbols(std::get<0>(Tuple), std::get<1>(Tuple));
----------------
ElfObjFile might be invalid. You will need to make another copy of the input, like in patch 1.


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

https://reviews.llvm.org/D65367