[PATCH] D62583: [llvm-objcopy] Implement IHEX reader

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 13 02:20:37 PDT 2019


jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:1
+# Check section headers when converting from hex to ELF
+# RUN: yaml2obj %p/Inputs/ihex-elf-sections.yaml -o %t
----------------
Nit: missing trailing full stop here and below at the end of comments. Also, we've started using '##' in many new LLVM tools tests to clarify the difference between comments and actual test commands, so it would be good if you changed that.


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:23
+# RUN: llvm-objcopy -I ihex -O elf32-i386 %p/Inputs/sections.hex %t-raw
+# RUN: llvm-readobj -section-headers %t-raw | FileCheck %s --check-prefix=RAW
+
----------------
Nit: don't mix single-dash long options with double-dash long options if you can avoid it (i.e. -section-headers -> --section-headers). Applies throughout this test.


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:192
+
+# BAD_LENGTH: error: '{{.*}}': line 1: line is too short: 10 chars
+# MISSING_COLON: error: '{{.*}}': line 1: missing ':' in the beginning of line
----------------
I'd find it much easier to read this test, if the CHECKs for each individual case appear immediately after the indivdiual test case(s) using them, i.e:


```
# RUN: some stuff | FileCheck
# CHECK:

# RUN: some more stuff | FileCheck --check-prefix=CHUCK
# RUN: yet more stuff | FileCheck --check-prefix=CHUCK
# CHUCK:
```


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

https://reviews.llvm.org/D62583





More information about the llvm-commits mailing list