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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
