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

Tue May 16 12:57:01 PDT 2023

MaskRay added inline comments.

================
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:
> 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).
I agree that `%T` should be avoided. If another test in `.../COFF/` does something with `%T`, this will easily cause a race condition. If a test file creates a lot of temporary files, sometimes `# RUN: rm -rf %t && mkdir %t && cd %t` is easier.

`--check-prefix=` is the most common form. ` -check-prefix=` is less recommended but common as well.
` -check-prefix ` is very uncommon and probably should be avoided.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150305