[PATCH] D65372: [llvm-objcopy] Add support for response files in llvm-strip and llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Hi @mmpozulp,

When you've addressed review comments, it's helpful if you mark the "Done" box in the relevant comment. That way it's obvious what hasn't been addressed. If you do it before you upload, then they will get submitted when the patch gets updated.

> It might be worth a test showing that "@FILE" appears when --help is explicitly passed on the command-line however.

Having thought about it, I definitely think I'd like this please. If it's not in the help text, it might as well not exist. You also need to update the documentation to show that it is supported (see similar changes I made recently to other tools).



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/response-file.test:7-8
+
+# RUN: llvm-objdump -h %t.o | FileCheck %s
+# RUN: llvm-objdump -h %t2.o | FileCheck %s
+
----------------
This is a minor point, but we've tended to use llvm-readelf/llvm-readobj to check sections, IIRC. Also, it's probably worth doing a cmp to show that the two output files are identical.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:11
 #include "Buffer.h"
+#include "COFF/COFFObjcopy.h"
 #include "CopyConfig.h"
----------------
Could you do this header reordering in a separate NFC commit please (no need to get a review).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65372