[PATCH] D120486: [objcopy] Refactor CommonConfig to add posibility to specify added/updated sections as MemoryBuffer.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:563
+// extracts section name and name of the file keeping section data from
+// ArgValue, loads data from the file, store section name and section data
+// into the vector of new sections \p NewSections.
----------------



================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:123
+    std::function<bool(const Binary &File)> IsValidFormat,
+    StringRef NewSectionName, StringRef NewSectionData, bool Add) {
+  auto ErrHandler = [&](const Twine &Msg) { FAIL() << "Error: " << Msg; };
----------------
I have a marginal prefernce to make `Add` an enum, as it helps give a bit more meaning to the parameter.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:163
+
+  // Check that copied file has new section.
+  bool HasNewSection = false;
----------------



================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:170
+
+    if (*SectNameOrErr == NewSectionName) {
+      HasNewSection = true;
----------------
Add a `break` at the end of the if.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:174-179
+      ASSERT_TRUE(Sect.getSize() == NewSectionData.size());
+      ASSERT_TRUE(memcmp(SectionData->data(), NewSectionData.data(),
+                         NewSectionData.size()) == 0);
+    }
+  }
+  ASSERT_TRUE(HasNewSection);
----------------
These can all be `EXPECT` rather than `ASSERT`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120486