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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits