[PATCH] D102665: [llvm-objcopy] Add support for '--' for delimiting options from input/output files

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/objcopy-dash-dash.test:1
+# Create a clean temporary directory and cd to it to use relative paths.
+# RUN: rm -rf %t && mkdir %t && cd %t
----------------
Nit: in newer llvm-objcopy tests, we tend to use `##` for comment markers, to make the comments stand out clearly from the test directives.

Also, no need for "objcopy" to be in the name. `dash-dash.test` would be sufficient. Since the functionality for llvm-strip would be nearly identical, it could live in the same test.


================
Comment at: llvm/test/tools/llvm-objcopy/objcopy-dash-dash.test:4-5
+
+# Check that llvm-objcopy works correctly with input/output file names staring
+# with dash separated by '--' from options.
+# RUN: yaml2obj %s -o -in-obj
----------------
I would change the commnet slightly to make it generic - after all, non-dashed files are still valid after this point too. Something like:

"Check that llvm-objcopy correctly treats paths after "--" as file paths, even if they start with dashes."

You would then have a case where the name has a dash as a leading character, and another where it doesn't. I'd make the filenames with a leading dash valid options too.

Also, I recommend you test the following cases:
1) `--` is specified with options but no input files.
2) `--` is specified with input files but no options.


================
Comment at: llvm/test/tools/llvm-objcopy/objcopy-dash-dash.test:8
+# RUN: llvm-objcopy --only-section=.test -- -in-obj -out-obj
+# RUN: llvm-readobj --file-headers --sections -- -out-obj | FileCheck %s
+
----------------
You don't need to check the full file properties. In fact, I'd recommend doing a run without `--` and then use `cmp` to compare this with the output of the different test cases.


================
Comment at: llvm/test/tools/llvm-objcopy/objcopy-dash-dash.test:10-19
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
----------------
This can be dramatically simplified. We don't need a section at all, and I believe the Machine is optional too. You can also remove excess padding, and add the `---` to properly start the YAML.


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

https://reviews.llvm.org/D102665