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

[Adrian McCarthy via lldb-commits]

amccarth added a comment.

Are we putting this code in the right place?  I wouldn't expect minidump parsing to fall under Plugins/Process.

I assume the eventual intent is to turn the Windows-specific code into a user of your new code?  I look forward to seeing that happen.


================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:13
@@ +12,3 @@
+#include "MinidumpParser.h"
+#include "MinidumpTypes.h"
+
----------------
Include MinidumpTypes.h first in MinidumpTypes.cpp.  This helps ensure that headers can stand alone.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:14
@@ +13,3 @@
+// C includes
+#include <cstring>
+
----------------
Maybe I'm missing it, but it doesn't seem like this header is using anything from `<cstring>` (which should be under C++ includes).

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:40
@@ +39,3 @@
+
+enum MinidumpStreamType
+{
----------------
    // Reference:  https://msdn.microsoft.com/en-us/library/windows/desktop/ms680394.aspx

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:257
@@ +256,3 @@
+};
+const int MINIDUMP_SYSTEM_INFO_SIZE = 3 * 2 + 2 * 1 + 4 * 4 + sizeof(RVA) + 2 * 2 + MINIDUMP_CPU_INFO_SIZE;
+static_assert(sizeof(MinidumpSystemInfo) == MINIDUMP_SYSTEM_INFO_SIZE, "sizeof MinidumpSystemInfo is not correct!");
----------------
Why do the arithmetic and static_assert?  Why not use `sizeof(MinidumpSystemInfo)`?


https://reviews.llvm.org/D23545