jhenderson added a comment. Can't comment too much on the file format details, but I've made some more general comments.
FYI, I'll be away from end of day Wednesday for 2 and a half weeks, so won't be able to further review after that point until I'm back. ================ Comment at: include/llvm/BinaryFormat/Minidump.h:81 +#include "llvm/BinaryFormat/MinidumpConstants.def" + LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue = */ 0xffffffffu), +}; ---------------- I believe if you format this line as: ``` LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/0xffffffffu), ``` clang-format will leave it unedited. I believe it has special rules for `/*<Name>=*/<value>` to label parameters. ================ Comment at: lib/Object/Minidump.cpp:58 +MinidumpFile::getMemoryInfoList() const { + auto OptionalStream = getRawStream(StreamType::MemoryInfoList); + if (!OptionalStream) ---------------- 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. ================ Comment at: lib/Object/Minidump.cpp:66 + const minidump::MemoryInfoListHeader &H = ExpectedHeader.get()[0]; + auto ExpectedData = getDataSlice(*OptionalStream, H.SizeOfHeader, + H.SizeOfEntry * H.NumberOfEntries); ---------------- Ditto. ================ Comment at: unittests/Object/MinidumpTest.cpp:617-618 + // MemoryInfoListHeader + 16, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry + 1, 0, 0, 0, // ??? + }; ---------------- I might make the data here be of size 15 to test the edge case. It's probably also worth a test case where the header size as specified by SizeOfHeader fits in the data but is smaller than the expected value. ================ Comment at: unittests/Object/MinidumpTest.cpp:634 + // MemoryInfoListHeader + 16, 0, 0, 0, 52, 0, 0, 0, // SizeOfHeader, SizeOfEntry + 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries ---------------- I might go for a value of 49 to test the edge value here. 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