[PATCH] D65040: [llvm-objcopy] Add support for --add-section for COFF

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks. Just a few minor things mostly in the test left.



================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:3
+
+# Test that obbjcopy adds section to the given object with expected contents.
+# RUN: echo DEADBEEF > %t.sec
----------------
A number of newer tests in the tools use '##' to distinguish comments from lit/FileCheck commands. Please update this and the other comments accordingly.

Also "objcopy" -> "llvm-objcopy", "adds section" -> "adds a section".


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:8
+
+# Test that objcopy produces an error if file with section contents to be added
+# does not exist.
----------------
"if file" -> "if the file"


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:13
+# Another negative test for invalid --add-sections command line argument.
+# RUN: not llvm-objcopy --add-section=.another.section %t2 %t %t3 2>&1 | FileCheck %s --check-prefixes=CHECK-ERR2
+
----------------
This isn't quite testing what it should be testing, since the command-line parser is interpreting %t2 as a positional argument. You should add just remove %t2 from your command-line here, and then you should get the error for the bad format of --add-section.


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:15
+
+# CHECK-ADD:      SectionCount: 2
+# CHECK-ADD:      Name: .text
----------------
I find it easier to follow the test if the FileCheck CHECK lines are immediately following the test case they are used for, so it would look something like:

```
# RUN: ...

# CHECK1: ...
# CHECK1: ...

# RUN: ...

# CHECK2: ...
# CHECK2: ...
```


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:26
+
+# CHECK-ERR1: llvm-objcopy{{(.exe)?}}: error{{.+}} No such file or directory
+# CHECK-ERR2: llvm-objcopy{{(.exe)?}}: error{{.+}} too many positional arguments
----------------
Can you use FileCheck's -D option here to check the file name in the error message? 

```
RUN: ... FileCheck -DFile=%t2

CHECK: ... error: [[FILE]] ...
```


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:38
+    SectionData:     488B0500000000C3
+symbols:
+...
----------------
Does yaml2obj allow this line to be deleted?


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:95
+
+  auto Contents = createGnuDebugLinkSectionContents(DebugLinkFile);
+
----------------
You can delete the empty lines before and after this line. Also, what is the type of Contents here? LLVM style is to not use auto in this sort of situation, as the type is not obvious from context.


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

https://reviews.llvm.org/D65040