[PATCH] D101332: [llvm-objcopy] Exclude empty sections in IHexWriter output

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry for the delay - I was off work last week. Some minor comments, otherwise basically this is good.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/ihex-writer-empty-sections.test:34
+
+!ELF
+FileHeader:
----------------
Whilst not required by yaml2obj, it is useful to have this leading `---`, because it is required when there are multiple YAML documents in the same file. Adding it now will allow a future expansion of the test with a different YAML input without needing to change this line.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/ihex-writer-empty-sections.test:40
+  Machine:         EM_ARM
+
+Sections:
----------------
Nit: we don't usually leave blank lines within the YAML, as it may make it less clear that this is all one YAML document.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/ihex-writer-empty-sections.test:49
+    Address:          0x33330000
+    AddressAlign:     0x1
+    Size:             0
----------------
I think you can omit the `AddressAlign` field throughout this YAML.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/ihex-writer-empty-sections.test:57
+    AddressAlign:     0x1
+    Content:          "000102030405060708090A0B0C0D0E0F"
+## An empty section that's placed at the end of .data0. This won't
----------------
I'm guessing the exact content isn't really important, in which case, I'd just go with something that's 2 or 4 bytes long for brevity (e.g. "0123"). Same goes elsewhere.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/ihex-writer-empty-sections.test:82
+    Content:          "00102030405060708090A0B0C0D0E0F0"
+## An empty section declared after .data1, but placed at .data1. This
+## won't be in the output.
----------------
I think this is a little clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101332