[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:153-155
+    if (auto Err = getContinuousData(Record, Length, 60, CompleteData))
+      return Err;
+    return Error::success();
----------------



================
Comment at: llvm/include/llvm/Object/GOFF.h:184
+  static Error getData(const uint8_t *Record, SmallString<256> &CompleteData) {
+    uint16_t dataSize = getNameLength(Record);
+    if (auto Err = getContinuousData(Record, dataSize, 72, CompleteData))
----------------
Why did you change the name casing here? LLVM style is UpperCamelCase.


================
Comment at: llvm/include/llvm/Object/GOFF.h:185-187
+    if (auto Err = getContinuousData(Record, dataSize, 72, CompleteData))
+      return Err;
+    return Error::success();
----------------



================
Comment at: llvm/include/llvm/Object/GOFF.h:415-417
+    if (auto Err = getContinuousData(Record, Length, 26, CompleteData))
+      return Err;
+    return Error::success();
----------------



================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:77
+
+    // If previous record was continued, the current record should be a
+    // continuation.
----------------



================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:190-192
+  if (auto Err = ESDRecord::getData(getSymbolEsdRecord(Symb), SymbolName)) {
+    return std::move(Err);
+  }
----------------
No need for braces for single-line if.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:349-350
+  }
+  return createStringError(llvm::errc::invalid_argument,
+                           "no symbol section found");
+}
----------------
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).


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:261
+          MemoryBufferRef(Data, "dummyGOFF"));
+  ASSERT_THAT_EXPECTED(
+      GOFFObjOrErr,
----------------
Nit: this could be `EXPECT_THAT_EXPECTED`. The test doesn't need to abort, if this check fails.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:311
+    Expected<StringRef> SymbolNameOrErr = GOFFObj->getSymbolName(Symbol);
+    ASSERT_THAT_EXPECTED(SymbolNameOrErr,
+                         FailedWithMessage("continued bit should not be set"));
----------------
This can be `EXPECT_THAT_EXPECTED`, right? There's no need to stop this test if it has a problem in this case, I'd think.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:454
+    Expected<SymbolRef::Type> SymbolType = Symbol.getType();
+    ASSERT_THAT_EXPECTED(
+        SymbolType,
----------------
As above.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:495
+    Expected<SymbolRef::Type> SymbolType = Symbol.getType();
+    ASSERT_THAT_EXPECTED(
+        SymbolType,
----------------
As above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437