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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The majority of your code is still untested as far as I can see. There appear to be three test cases you have so far:

1. An invalid size for a GOFF object.
2. A valid size for a GOFF object.
3. That `getSymbolName` returns the name of a single symbol in the symbol table.

What about all the rest of the functionality that is included in this patch, including, but certainly not limited to, the following?

1. More than one symbol in the symbol table.
2. Other properties of symbols.
3. The various properties of records.
4. Relocations.
5. And so on...

For each bit of code you have written, consider whether a test would fail if that bit of code was broken in some way, or didn't exist. If no test fails, then that code needs a new test case of some form. There may also be other cases where testing is appropriate, e.g. where two separate aspects of the same system interact in some way, although those are harder to judge.



================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:65-66
+    // a previous record parse call.
+    if (IsContinuation)
+      continue;
+
----------------
I don't see a test case involving a continuation record. You should have one followed by a non-continuation record, as otherwise this aspect is not tested.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:21
+
+void initializeGOFFData() {
+  // HDR record.
----------------
This and the other functions below are only used in one place, if I'm not mistaken. As such, just inline them - splitting them off makes it harder to follow what the individaul tests are doing, since you have to jump around the file.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:44
+void constructValidGOFF() {
+  StringRef ValidSize(GOFFData, 80);
+  Expected<std::unique_ptr<ObjectFile>> GOFFObjOrErr =
----------------
The valid size can be any multiple of 80 bytes. I'd recommend a second test-case that uses a size of something other than 80 bytes, e.g. 160 bytes.

What about 0 bytes? That probably needs a specific test case, as that is a multiple of 80...


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:53
+void constructInvalidGOFF() {
+  // Construct GOFFObject with record of length != 80.
+  StringRef InvalidData(GOFFData, 70);
----------------
According to the code, it needs to be a multiple of 80 bytes, so this comment isn't quite correct (it implies 160 is not a valid size).


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:54
+  // Construct GOFFObject with record of length != 80.
+  StringRef InvalidData(GOFFData, 70);
+  Expected<std::unique_ptr<ObjectFile>> GOFFObjOrErr =
----------------
Test the edge cases e.g. 79 and/or 81.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:59
+
+  ASSERT_THAT_EXPECTED(GOFFObjOrErr, Failed());
+}
----------------
Rather than `Failed()`, use `FailedWithMessage()`, so that you can check the error message output.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:74
+
+  auto Symbols = GOFFObj->symbols();
+  for (const SymbolRef &Symbol : Symbols) {
----------------
This is only used once - just inline it.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:75
+  auto Symbols = GOFFObj->symbols();
+  for (const SymbolRef &Symbol : Symbols) {
+    Expected<StringRef> SymbolNameOrErr = GOFFObj->getSymbolName(Symbol);
----------------
I suspect, given the name, that the `SymbolRef` type is very small already (in the same manner as `StringRef`), and there's no real benefit in making this a `const &`.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:80-81
+
+    ASSERT_EQ(SymbolName.empty(), false);
+    EXPECT_STREQ(SymbolName.data(), "Hello");
+  }
----------------
You should just be able to do `EXPECT_EQ(SymbolName, "Hello");` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071