[PATCH] D55881: [llvm-objcopy] [COFF] Add support for removing symbols

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/COFF/Inputs/remove-symbols.yaml:38
+
+--- !COFF
+header:          
----------------
Similar to my comments in D56294, how much of this YAML is actually necessary to test symbol removal? We really only want the very minimum needed for this particular case (and we can add more for other tests or too this test as we add more cases).


================
Comment at: test/tools/llvm-objcopy/COFF/remove-symbol1.test:1
+# RUN: yaml2obj %p/Inputs/remove-symbols.yaml > %t.o
+
----------------
Please rename this to strip-symbol.test, to match the long form of the switch name. Also, please add a case using the long form. This could be a second run of llvm-objcopy on the same input, and show that you get the same output. Finally, the general pattern in most ELF tests is to make the edits out-of-line (i.e. specify a different output file to the input file), and then to show that the input file was not itself modified by the operation. This last point is probably only important because of the second point re. the long form switch name etc.


================
Comment at: test/tools/llvm-objcopy/COFF/remove-symbol2.test:1
+# RUN: yaml2obj %p/Inputs/remove-symbols.yaml > %t.o
+# RUN: not llvm-objcopy -N ptr %t.o 2>&1 | FileCheck %s --check-prefix=ERROR
----------------
Rename this test to something meaningful, e.g. strip-referenced-symbol.test

In fact, I wonder if we should mirror the equivalent ELF test (strip-reloc-symbol.test) and error message as much as possible? What do you think?


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

https://reviews.llvm.org/D55881