[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:598
       !Common.SymbolsToAdd.empty()) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for MachO");
----------------
mstorsjo wrote:
> On the topic of testing diagnostics... Here (in the `get*Config()` format specific methods) we do error out if a common option has been set, that isn't implemented for that format. But what if options in one of the different format specific structs have been set, e.g. an option set in `ELF`, but we end up calling `getMachOConfig()` - then we don't error out at all.
> 
> Should we add a separate `bool ELFOptionsSet` (and similar for all other formats) which we could check for in the `get*Config()` methods, so we error out cleanly in the same way, if format specific options are set for the wrong format?
Personally I think no action item is needed for you on this one:)

The diagnostic is untested... I remember that when we add recent new binary-format-specific options, we don't bother add tests for other binary formats. I would not bother adding a diagnostic for non-COFF if I am to implement the feature:)

This part may use some refactoring which can make such tests less necessary, but that will be out of the scope of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116556