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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/pe-fields.test:7
+
+# CHECK: unable to set subsystem on an object file
+
----------------
Should the terminology "object files" actually be "relocatable object files"? At least for ELF, al ELF files are object files, whether executable or not (I don't know if the same applies for COFF though).

Regardless, it would be good if this error message contained the offending file's name.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/subsystem.test:25
+
+# INVALID-SUBSYS: is not a valid subsystem
+
----------------
This check should include the "foobar" in it somewhere. You might want a `{{$}}` at the end of this message too, since it's a strict prefix of one of the other errors (and you want to avoid a false pass, where the test is reporting a differenet error).


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/subsystem.test:30
+
+# INVALID-NUMBER: is not a valid subsystem {{.*}} version
+
----------------
I'd prefer it if this had the explicit "major" or "minor" version. Could be achieved using FileCheck variables, i.e. `is not a valid subsystem [[TYPE]] version` or similar.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:263
+    if (!Obj.IsPE)
+      return createStringError(llvm::errc::invalid_argument,
+                               "unable to set subsystem on an object file");
----------------
At least elsewhere in the patch, we don't bother with the explicit `llvm::` prefix here.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:738
 
+  for (auto Arg : InputArgs.filtered(OBJCOPY_subsystem)) {
+    StringRef Subsystem, Version;
----------------
mstorsjo wrote:
> jhenderson wrote:
> > Probably should be spelled out rather than using `auto`? In particular, I believe it might be a pointer, so this needs clarifying.
> I can change this into `for (const llvm::opt::Arg *Arg`. Note that there’s many other identical loops with `for (auto Arg : InputArgs.filtered(…` within the same function, so this makes it a bit more inconsistent though.
I'd be okay with `const auto *Arg` if you prefer that for a degree of additional consistency. My main issue is the pointer-ness of `Arg`. Or, if you are particularly keen on the consistency, I don't mind reverting to what you had before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116556