[PATCH] D103490: [SystemZ][z/OS] Add support for TXT records in the GOFF reader

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:36
   IndexedMap<const uint8_t *> EsdPtrs; // Indexed by EsdId.
+  SmallVector<const uint8_t *, 256> TextPtrs;
 
----------------
yusra.syeda wrote:
> jhenderson wrote:
> > Any particular reason you've chosen the size 256 here and in various other places? It may deserve a comment or some sort of named constant.
> An initial vector size of 256 guarantees that in most cases the vector does not need to be extended, and avoids a memory allocation in SmallVector. 
The trade-off is that this requires a significantly bigger stack allocation than might be necessary. How many TextPtrs might a typical file have?


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:519-520
+
+  StringRef Res = StringRef(Data);
+  return arrayRefFromStringRef(Res);
+}
----------------
jhenderson wrote:
> Why not simply `return arrayRefFromStringRef(StringRef(Data));`? In fact, could you get away with returning simply `Data.data()` or similar?
> In fact, could you get away with returning simply Data.data() or similar?

Any response to this part of the comment? I'm pretty sure you could just do `return {Err.data(), Err.size()}` without needing to jump through the StringRef constructor.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:543-544
+  ESDRecord::getExecutable(EsdRecord, Executable);
+  bool IsSectionData = (Executable == GOFF::ESD_EXE_DATA);
+  return IsSectionData;
+}
----------------
As previously requested in my above comment ("same below...") don't add unnecessary local variables like this.

Really, I'd expect this return line to just be `return ESDREcord::getExecutable(EsdRecord) == GOFF::ESD_EXE_DATA;`. I don't know why you have several functions that return via input parameter rather than normal C++ way of using the return type...


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:587
+
+  int SymbolNum = 0;
+  for (SymbolRef Symbol : GOFFObj->symbols()) {
----------------
`size_t` seems more appropriate for counting the number of something.


================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:593
+    EXPECT_EQ(SymbolName, "var#c");
+    SymbolNum ++;
+  }
----------------
Prefer pre to postincrement. However, I think I have a beter suggestion, if you only expect one symbol:

```
auto Symbols  = GOFFObj->symbols();
ASSERT_EQ(std::distance(Symbols.begin(), Symbols.end(), 1);
SymbolRef Symbol = *Symbols.begin();
Expected<StringRef> SymbolNameOrErr = GOFFObj->getSymbolName(Symbol);
ASSERT_THAT_EXPECTED(SymbolNameOrErr, Succeeded());
StringRef SymbolName = SymbolNameOrErr.get();
EXPECT_EQ(SymbolName, "var#c");
```

A similar change would work for the section check below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103490