zturner added inline comments. ================ Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67 @@ +66,3 @@ + bool m_valid_data; + llvm::DenseMap<uint32_t, MinidumpLocationDescriptor> m_directory_map; +}; ---------------- Can this be `llvm::DenseMap<MinidumpStreamType, MinidumpLocationDescriptor>`?
No point erasing the type information of the enum. ================ Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21 @@ +20,3 @@ + +llvm::Optional<const MinidumpHeader *> +MinidumpHeader::Parse(llvm::ArrayRef<uint8_t> &data) ---------------- I think these can all just return the pointer instead of `llvm::Optional<>`. return `nullptr` to indicate failure. Optionally, make the signature be something like `Error MinidumpHeader::Parse(llvm::ArrayRef<uint8_t> &data, const MinidumpHeader *&Header)` which would allow you to propagate the error up (if you wanted to). At the very least though, there's no point using `llvm::Optional<>` if `nullptr` can be used to indicate failure. ================ Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:30 @@ +29,3 @@ + const MinidumpHeaderConstants version = + static_cast<const MinidumpHeaderConstants>(static_cast<const uint32_t>(header->version) & 0x0000ffff); + ---------------- Where does the `0xFFFF` come from? ================ Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:133 @@ +132,3 @@ +// This matches the Linux kernel definitions from <asm/hwcaps.h> +enum MinidumpPCPUInformationARMElfHwCaps +{ ---------------- Should this be `enum class`? https://reviews.llvm.org/D23545 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits