[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);
----------------
jhenderson wrote:
> Any particular reason you've used `(char)0x00` rather than simply `\0`?
Ping this comment?


================
Comment at: llvm/include/llvm/Object/GOFF.h:39
+
+  static Error GetContinuousData(const uint8_t *Record, uint16_t DataLength,
+                                 int DataIndex, SmallString<256> &CompleteData);
----------------
Please keep the old naming scheme. If you aren't familiar with it, check out the LLVM coding guidelines (https://llvm.org/docs/CodingStandards.html), which includes documentation on naming functions and variables (functions lowerCamelCase, variables UpperCamelCase being the gist).


================
Comment at: llvm/include/llvm/Object/GOFF.h:125
+
+  static Error GetData(const uint8_t *Record, SmallString<256> &CompleteData) {
+    uint16_t Length = getPropertyModuleLength(Record);
----------------
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.


================
Comment at: llvm/include/llvm/Object/GOFF.h:155
+
+  static Error GetData(const uint8_t *Record, SmallString<256> &CompleteData) {
+    uint16_t dataSize = getNameLength(Record);
----------------
Ditto.


================
Comment at: llvm/include/llvm/Object/GOFF.h:383
+
+  static Error GetData(const uint8_t *Record, SmallString<256> &CompleteData) {
+    uint16_t Length = getNameLength(Record);
----------------
Ditto.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:190
+  SmallString<256> SymbolName;
+  if (auto Err = ESDRecord::getData(getSymbolEsdRecord(Symb), SymbolName))
+    return std::move(Err);
----------------
Have you built this code? This error looks like it would break the build...


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:437
+       DataLength -= SliceLength, Slice += GOFF::PayloadLength) {
+    // Slice points to the begin of the new record.
+    // Check that this block is a Continuation.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437