[PATCH] D124804: [Object][DX] Parse DXContainer Parts

[Chris Bieneman via Phabricator via llvm-commits]

beanz added a comment.

libObject uses MemoryBufferRefs which play back and forth with StringRef, and while it has been pointed out over and over again that many of StringRef’s methods should probably be on MemoryBufferRef because they are more broadly useful, it is the way it is.

I think ArrayRef<char> is probably the least common pattern for representing arbitrary data buffers in LLVM, and given the needs of libObject to operate with MemoryBufferRef I think this code is best to operate on StringRefs.



================
Comment at: llvm/lib/Object/DXContainer.cpp:34
+template <typename T>
+static Error readValue(StringRef Buffer, const char *P, T &Val) {
+  // Don't read before the beginning or past the end of the file
----------------
kuhar wrote:
> Can you give `P` a more descriptive name and/or add a function comment?
> Also, why not return `Expected<T>`
Mostly just because the usage pattern for `Error` is cleaner in this case, but also it avoids a copy and is in-line able.

With Expected the code would be:
```
auto ExVal = readValue(…);
if (!ExVal)
  return ExVal.takeError();
Val = *ExVal;
```

Which is a bit less clean to me.


================
Comment at: llvm/lib/Object/DXContainer.cpp:76-77
+void DXContainer::PartIterator::updateIterator() {
+  if (OffsetIt == Container.PartOffsets.end())
+    return;
+  StringRef Buffer = Container.Data.getBuffer();
----------------
kuhar wrote:
> Would it be possible for `OffsetIt` to equal the end iterator and end up with an uninitialized `IteratorState`?
It shouldn’t be, but looking through I think there’s an edge case. I will update with a fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124804