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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:50
+public:
+  std::error_code getSymbolName(SymbolRef Symbol, StringRef &Res) const;
+
----------------
Why is `getSymbolName` still returning a `std::error_code` and not an `Error`?


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:47
+    Err = createStringError(object_error::unexpected_eof,
+                            "object file is not the right size.");
+    return;
----------------
What is the right size? What is the size that the object file actually is? Please include context in the message. Also, no trailing full stop in error messages. See https://llvm.org/docs/CodingStandards.html#error-and-warning-messages.


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

begin = verb, beginning/start = nouns


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:290
+        std::errc::invalid_argument,
+        "symbolType must be SectionDef/ElemDef/LabelDef/PartRef/ExtRef.");
+
----------------
What has actually been specified though? Which symbol (if known)?


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:303
+      return createStringError(std::errc::invalid_argument,
+                               "executable must be CODE/DATA/Unspecified.");
+    switch (Executable) {
----------------
What type is it actually?

Also, clang-format.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:338
+  }
+  llvm_unreachable("unable to get symbol section");
+  return section_iterator(SectionRef(Sec, this));
----------------
yusra.syeda wrote:
> jhenderson wrote:
> > Is this really unreachable? What happens if there is no symbol section in the file?
> Yes, this should be unreachable and is added as an extra safety check.
You didn't answer my question: "What happens if there is no symbol section in the file?"


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:75
+
+TEST(GOFFObjectFileTest, ConstructGOFFObject) {
+  constructAndGetSymbols();
----------------
clang-format this file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071