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

Mon May 15 00:08:20 PDT 2023

jhenderson 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.

What was the desperate rush about getting this landed without giving it at least 1 working day for other reviewers to have a final check through, especially when you actively disagreed with one of the requests?

================
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
+
----------------
compnerd wrote:
> 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.
The parent of the source will be a directory, which is all you're trying to achieve here. The docs state that using %T is deprecated. You shouldn't use it in new tests. Please address this point.

It looks like you didn't adopt my inline request to use double-dash for --check-prefix: as things stand, you've got FileCheck commands in this test using both single and double-dash (I'm pretty sure double-dash is preferred everwhere).

================
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
----------------
compnerd wrote:
> 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.
This comment was meant to apply to both error cases: please update CHECK-NO-SECTION too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150305