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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've done a quick skim and picked up some nits, but unfortunately, I don't have time to properly review the tests here, and am unlikely to do so for quite some time.



================
Comment at: llvm/include/llvm/Object/GOFF.h:250
+
+    for (int I = 0; I < Length; ++I)
+      set<uint8_t>(72 + I, Data[I]);
----------------
`Length` is `uint8_t`, so use `uint8_t` for `I` too.


================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:35
+
+  mutable DenseMap<uint32_t, std::string> EsdNames;
+
----------------
I'm trying to understand why `EsdNames` and `SectionData` are `mutable`. Are they intended as caches that are lazily populated? If so, I'd probably make that clear from the names (e.g. `EsdNamesCache` and `SectionDataCache`).


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:59
+
+    // Don't parse continuations records, only parse initial record
+    if (IsContinuation)
----------------



================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:21
+
+void constructValidGOFF(int size) {
+  StringRef ValidSize(GOFFData, size);
----------------
Why is `size` an `int` and not a `size_t`? Also it should be `Size`. Same below.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:40
+      FailedWithMessage("object file is not the right size. Must be a multiple "
+                        "of 80 bytes, but is % bytes"));
+}
----------------
"% bytes"? That doesn't look right...


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:158
+  GOFFData[GOFF::RecordLength + 3] = 0x00;
+  GOFFData[GOFF::RecordLength + 7] = 0x01; // ESDID.
+  GOFFData[GOFF::RecordLength + 71] = 0x01; // Size of symbol name.
----------------
Please make suer to reformat all your new code, by running clang-format.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:192
+    StringRef SymbolName = SymbolNameOrErr.get();
+    ASSERT_EQ(SymbolName.empty(), false);
+    EXPECT_EQ(SymbolName, "Hello");
----------------
I don't think you need this `ASSERT_EQ` here. The `EXPECT_EQ` code should be able to handle an empty string. Same goes in other places in other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437