[PATCH] D116556: [llvm-objcopy] Implement the PE-COFF specific --subsystem option

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:758
+      return createStringError(errc::invalid_argument,
+                               "'%s' is not a valid subsystem",
+                               Subsystem.str().c_str());
----------------
The diagnostic should be tested.

`ELF/` has relatively better testing. See `ELF/add-section.test` for an example that various cases are tested.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:762
+      StringRef Major, Minor;
+      std::tie(Major, Minor) = Version.split('.');
+      unsigned Number;
----------------
A version without `.` should be tested.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:766
+        return createStringError(errc::invalid_argument,
+                                 "'%s' is not a valid subsystem major version",
+                                 Major.str().c_str());
----------------
The diagnostic should be tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116556