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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/pe-fields.test:7
+
+# CHECK: unable to set subsystem on an object file
+
----------------
jhenderson wrote:
> 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.
I guess relocatable object file is more correct for COFF too. I'll include the offending file name and check it in the test.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/subsystem.test:25
+
+# INVALID-SUBSYS: is not a valid subsystem
+
----------------
jhenderson wrote:
> 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).
Ok, I'll include the incorrect subsystem name in the test pattern. (Then it's no longer a prefix of the other messages, but I can include the `{{$}}` for good measure in any case.)


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/subsystem.test:30
+
+# INVALID-NUMBER: is not a valid subsystem {{.*}} version
+
----------------
jhenderson wrote:
> 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.
Ok. I can just split up the patterns for clarity too, and have the check include the unparseable part of the version number too.


================
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");
----------------
jhenderson wrote:
> At least elsewhere in the patch, we don't bother with the explicit `llvm::` prefix here.
Ok, I'll leave it out here. (This file has a couple existing cases with such redundant prefixes.)


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:738
 
+  for (auto Arg : InputArgs.filtered(OBJCOPY_subsystem)) {
+    StringRef Subsystem, Version;
----------------
jhenderson wrote:
> 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.
Yeah `const auto *Arg` looks fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116556