[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.


================
Comment at: lib/Object/Minidump.cpp:58
+MinidumpFile::getMemoryInfoList() const {
+  auto OptionalStream = getRawStream(StreamType::MemoryInfoList);
+  if (!OptionalStream)
----------------
labath wrote:
> jhenderson wrote:
> > I probably should have picked up on this in previous reviews, but this is too much `auto` for my liking, as it's not obvious from the call site what `getRawStream` returns.
> Done. I've also changed the other calls to getRawStream.
Thanks!


================
Comment at: unittests/Object/MinidumpTest.cpp:620
+  };
+  EXPECT_THAT_EXPECTED(cantFail(create(HeaderTooBig))->getMemoryInfoList(),
+                       Failed<BinaryError>());
----------------
Here and in the similar places, I'm not convinced that `cantFail` is appropriate (if the creation code is broken, this will assert and therefore possibly hide the actual testing failures that show where it went wrong more precisely). It should probably be a two phase thing:

```
Expected<std::unique_ptr<MinidumpFile>> Minidump = HeaderTooBig);
ASSERT_THAT_EXPECTED(Minidump, Succeeded());
EXPECTE_THAT_EXPECTED(Minidump->getMemoryInfoList(), Failed<BinaryError>());
```


================
Comment at: unittests/Object/MinidumpTest.cpp:624
+  // Header fits into the stream, but it is too small to contain the required
+  // entries).
+  std::vector<uint8_t> HeaderTooSmall{
----------------
Nit: delete the ')'


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68210