[PATCH] D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've not reviewed the testing yet, but my immediate thought is that there needs to be a lot more, handling all the different code paths.



================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:109
+
+enum ESDExecutable {
+  ESD_EXE_Unspecified = 0,
----------------
Is the field that holds this not a fixed size type? If it is, you could use `uint8_t`/`uin16_t` etc as appropriate here to match.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:292-293
+    return createStringError(llvm::errc::invalid_argument,
+                             "ESD record %lu has invalid symbol type %02X",
+                             (unsigned long)EsdId, SymbolType);
+  }
----------------
jhenderson wrote:
> Rather than casting `EsdId`, use the correct print format specifier.
Now that `SymbolType` is defined to be a `uint8_t`, you should use that explicitly, i.e. something like `0x%02" PRIX8"` (though you could probably simplify and omit the "02" bit, since this is an error message, and getting a fixed width field isn't really necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071