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

[Yusra Syeda via Phabricator via llvm-commits]

yusra.syeda marked 4 inline comments as done.
yusra.syeda 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;
 
----------------
jhenderson wrote:
> 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?
The amount of TXT records can vary quite a bit. In an analysis of about 3390 object files, the average was about 1200 with the minimum being 3 and maximum being 376780. So 256 is a reasonable value.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:519-520
+
+  StringRef Res = StringRef(Data);
+  return arrayRefFromStringRef(Res);
+}
----------------
jhenderson wrote:
> yusra.syeda wrote:
> > jhenderson wrote:
> > > 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.
> > Seems I can do something like this to avoid the StringRef constructor:
> > ```
> >   ArrayRef<uint8_t> temp(reinterpret_cast<const unsigned char *>(Data.data()), Data.size());
> >   return temp;
> > ```
> > If you prefer this, I can update it. 
> Ah, I see what some of the problem is now - didn't realise this returned an `ArrayRef<uint8_t>` rather than an `ArrayRef<char>`. Seems to me like this might be because your interfaces are inconsistent - some are using strings (effectively arrays of `char`), and others are using arrays of `uint8_t`). Is it possible to change your methods to canonicalise on one? To me it would be better to be the `uint8_t` option, since that is less likely to fall foul of signed conversion issues, and it's the closest thing to a true byte type that we have.
> 
> I think leaving as-is is fine, if it's not practical to canonicalise. For reference, you shouldn't need the temporary in your example, and should be able to just return it directly.
Thanks, I will leave this as is for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103490