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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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);
----------------
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?


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:83
+            object_error::parse_failed,
+            "continued record is not followed by a continuation record");
+        return;
----------------
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.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:442-443
+    if (DataLength <= 77 && Record::isContinued(Slice))
+      return createStringError(object_error::parse_failed,
+                               "continued bit should not be set");
+
----------------
Same as my other comments.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:349-350
+  }
+  return createStringError(llvm::errc::invalid_argument,
+                           "no symbol section found");
+}
----------------
jhenderson wrote:
> This message could be improved, I believe. If I understand it correctly, this error is trying to say that a symbol's section is not found, rather than that a "symbol section" is not found. I think you should record more information in the message, i.e. which symbol is missing its section, and what should identify that section (index/name/ID or whatever - I don't really follow the actual logic).
Ping this comment? (It was referring to the "no symbol section found" error message).

Tip: if you have not yet addressed a comment, but have plans to do so in a future update, say so when you update the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437