[PATCH] D150305: ObjCopy: support `--dump-section` on COFF

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd marked 2 inline comments as done.
compnerd added a comment.

I'll make those adjustments and then commit - I'd like to get this through.  I can address further feedback in follow ups as it seems most of the comments are about stylistic things at this point.



================
Comment at: llvm/test/tools/llvm-objcopy/COFF/dump-section.test:6-7
+# RUN: od -t x1 %t.txt | FileCheck %s --ignore-case --check-prefix CHECK-TEXT-F
+# RUN: not llvm-objcopy --dump-section non-existent=/dev/null %t.obj 2>&1 | FileCheck %s -check-prefix CHECK-NO-SECTION
+# RUN: not llvm-objcopy --dump-section .text=%T %t.obj 2>&1 | FileCheck %s --ignore-case -check-prefix CHECK-INVALID-DESTINATION
+
----------------
jhenderson wrote:
> Consistency fixes, and no need to ignore the case. Instead, use the %errc... substitutions, if needed (you might need to extend the list of available ones to cover the "is a directory" case).
> 
> Also `%T` is considered deprecated (from the docs: "%T 	parent directory of %t (not unique, deprecated, do not use)"). You can use `%S` instead in this case.
`%T` is the correct thing here; `%S` is the parent of the source which would write the file into the source tree.  Creating a directory with `%t` is possible but annoying.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/dump-section.test:13-15
+# CHECK-NO-SECTION: section 'non-existent' not found
+
+# CHECK-INVALID-DESTINATION: : is a directory
----------------
jhenderson wrote:
> Could you check the full error message, please (less any tool name), using FileCheck -D to parameterise any file names. In particular, this will show that this is an error, and not a warning.
Sure.


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

https://reviews.llvm.org/D150305