[PATCH] D70036: [llvm-objcopy][COFF] Implement --redefine-sym and --redefine-syms

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Looks okay to me, but I don't know enough about COFF to know whether there are any hidden gotchas with renaming symbols that might need accounting for.



================
Comment at: llvm/test/tools/llvm-objcopy/COFF/redefine-symbol.test:6-7
+
+# RUN: echo 'func cnuf #rename func' > %t.rename.txt
+# RUN: echo 'foo bar' >> %t.rename.txt
+# RUN: llvm-objcopy --redefine-syms %t.rename.txt %t %t3
----------------
Maybe add some leading whitespace to one of these two lines?


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/redefine-symbol.test:31
+# CHECK-NEXT: }
+
+--- !COFF
----------------
MaskRay wrote:
> `test/tools/llvm-objcopy/ELF/redefine-symbol.test` contains tests for incorrect option usage (generic). I guess we probably don't need to duplicate that.
We don't need to duplicate it, but perhaps we should move that bit into test/tools/llvm-objcopy (i.e. out of the ELF layer)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70036