[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/lib/Object/GOFFObjectFile.cpp:519-520
+
+  StringRef Res = StringRef(Data);
+  return arrayRefFromStringRef(Res);
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103490