[PATCH] D111437: [SystemZ/z/OS] Implement GOFF writer for empty files

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:82
+#ifndef NDEBUG
+  /// The number of bytes needed to fill up the last physical record.
+  size_t Gap = 0;
----------------
It might be worth explaining why this is in an ifdef, in this comment.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:139
+  // Support for endian-specific data.
+  template <typename value_type> void writebe(value_type Value) {
+    Value = support::endian::byte_swap<value_type>(Value, support::big);
----------------
"BE" being an acronym for "big endian" should be capitalized.


================
Comment at: llvm/lib/Target/SystemZ/MCTargetDesc/SystemZELFObjectWriter.cpp:58
   switch (Kind) {
   case FK_Data_2:                return ELF::R_390_PC16;
   case FK_Data_4:                return ELF::R_390_PC32;
----------------
jhenderson wrote:
> Whilst you're moving this file, I'd consider doing a complete clang-format of it too.
Not addressed?


================
Comment at: llvm/test/MC/GOFF/empty-goff.s:4-8
+* Header record:
+*  03 is prefix byte
+*  f. is header type
+*  .0 is version
+* The 1 at offset 51 is the architecture level.
----------------
For true comments, as opposed to lit or FileCheck directives, I like to use double comment markers, i.e. "** Header record:" etc. Also why "*"? "#" is the more common marker I've seen used (although I admit it doesn't apply everywhere).


================
Comment at: llvm/test/MC/GOFF/empty-goff.s:1
+* RUN: llvm-mc <%s --triple s390x-ibm-zos --filetype=obj -o - | \
+* RUN:   od -Ax -tx1 -v | FileCheck --ignore-case %s
----------------
jhenderson wrote:
> I'd find this syntax easier to follow, as it's a more common style among tests I've worked with.
Not addressed?


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

https://reviews.llvm.org/D111437