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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/ObjCopy/CommonConfig.h:193
+  NewSectionInfo() = default;
+  NewSectionInfo(StringRef Name, std::unique_ptr<MemoryBuffer> &Buffer) {
+    SectionName = Name;
----------------
Take the `Buffer` by R-value reference, since you are releasing its contents.


================
Comment at: llvm/include/llvm/ObjCopy/CommonConfig.h:194
+  NewSectionInfo(StringRef Name, std::unique_ptr<MemoryBuffer> &Buffer) {
+    SectionName = Name;
+    SectionData.reset(Buffer.release());
----------------
Use an initialiser list.


================
Comment at: llvm/include/llvm/ObjCopy/CommonConfig.h:199
+  StringRef SectionName;
+  std::shared_ptr<MemoryBuffer> SectionData;
+};
----------------
Why `shared_ptr`?


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:62-64
 # RUN: not llvm-objcopy --add-section=.another.section=%t2 %t %t3 2>&1 | FileCheck -DFILE1=%t -DFILE2=%t2 -DMSG=%errc_ENOENT %s --check-prefixes=ERR1
 
+# ERR1: llvm-objcopy: error: '[[FILE2]]': [[MSG]]
----------------
Here and throughout your test changes, please remove the unused `FILE1` variable, and rename `FILE2` to `FILE` (or make equivalent changes).


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:851-867
     StringRef ArgValue(Arg->getValue());
     if (!ArgValue.contains('='))
       return createStringError(errc::invalid_argument,
                                "bad format for --add-section: missing '='");
-    if (ArgValue.split("=").second.empty())
+    std::pair<StringRef, StringRef> SecPair = ArgValue.split("=");
+    if (SecPair.second.empty())
       return createStringError(
----------------
It would be great if you could pull all this logic into a function for sharing with the update section case. I think the only differences are the error messages and where the final data gets put.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:120
+
+TEST(AddUpdateDebugSection, ELF) {
+  const char *YamlCreationString = R"(
----------------
Split this into separate Add and Update test cases, to avoid confusing the two functionalities.

As the fact that the logic is independent of whether it is a debug section or not, remove reference to "debug" and use arbitrary section contents below (e.g. ".foo" with content "1234" or whatever), rather than a real debug section.

Why only an ELF case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120486