[PATCH] D81123: [llvm-objcopy] Reorder --dump-section for MachO. Fixes PR44283

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Mostly good



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/dump-section-before-add-remove.test:4
+## Verify that section is dumped before remove.
+# RUN: llvm-objcopy --dump-section __TEXT,__text=%t1.dump.text --remove-section __TEXT,__text %t %t2
+# RUN: FileCheck %s --input-file=%t1.dump.text --check-prefix=CONTENTS --implicit-check-not={{.}}
----------------
There is no need to use different numbering (%t1 %t2) for the same command.
Actually, it is preferred to use the same numbering for associated output (%t1.dump.text and %t2) in this case.

`.dump.text` can be simplified to `.dump` . You don't need two extensions.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:190
 
+  // Dump sections before add/remove for
+  // compatibility with GNU objcopy.
----------------
The two lines should be joined.

clang-format can format that for you.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:195
+    StringRef FileName;
+    std::tie(SectionName, FileName) = Flag.split("=");
+    if (Error E = dumpSectionToFile(SectionName, FileName, Obj))
----------------
`'='`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81123