[Lldb-commits] [PATCH] D23545: Minidump parsing

[Zachary Turner via lldb-commits]

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