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

[Pavel Labath via Phabricator via llvm-commits]

labath marked 2 inline comments as done.
labath added inline comments.


================
Comment at: unittests/Object/MinidumpTest.cpp:620
+  };
+  EXPECT_THAT_EXPECTED(cantFail(create(HeaderTooBig))->getMemoryInfoList(),
+                       Failed<BinaryError>());
----------------
jhenderson wrote:
> 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>());
> ```
Done. The previous solution of just passing the creation error in the helper function was totally bogus, of course. Ideally, it would be possible to express this as a one-liner using gtest matchers (`HasValue(Property(&getMemoryInfoList, Failed))`, but unfortunately they are quite incompatible with the move-only Expected semantices.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68210