[PATCH] D98437: [SystemZ][z/OS] Add GOFFObjectFile class support for HDR, ESD and END records

[Yusra Syeda via Phabricator via llvm-commits]

yusra.syeda marked 3 inline comments as done.
yusra.syeda added inline comments.


================
Comment at: llvm/include/llvm/Object/GOFF.h:35
+public:
+  Record() : Bytes(GOFF::RecordLength, (char)0x00) {
+    set<uint8_t>(0, GOFF::PTVPrefix);
----------------
jhenderson wrote:
> yusra.syeda wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > Any particular reason you've used `(char)0x00` rather than simply `\0`?
> > > Ping this comment?
> > Using `\0` causes a build failure.
> Were you using `'\0'` (with quotes)? If so, getting an error doesn't seem right. The return type of `(char)0x00` is identical to `'\0'`. What is the error that you are getting? What compiler version are you using?
I removed the setters from this file as they are not required for this patch. The need for this change no longer applies. 


================
Comment at: llvm/include/llvm/Object/GOFF.h:125
+
+  static Error GetData(const uint8_t *Record, SmallString<256> &CompleteData) {
+    uint16_t Length = getPropertyModuleLength(Record);
----------------
jhenderson wrote:
> Same comment as above. Also, these `SmallString` diagnostic errors might imply you're missing an include, so other usages of this header might have issues.
I'm not sure which include I'm missing. Shouldn't a missing include cause a build failure? 


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:83
+            object_error::parse_failed,
+            "continued record is not followed by a continuation record");
+        return;
----------------
jhenderson wrote:
> Similar to my other comments, this and the following error messages don't really provide enough context for a user to addresse them. See https://llvm.org/docs/CodingStandards.html#error-and-warning-messages for explanation and examples.
Thanks, I will add more details to this error message and the others you pointed out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437