[Lldb-commits] [PATCH] D12126: Read exception records from Windows mini dump

[Zachary Turner via lldb-commits]

zturner added a comment.

I would really like to start seeing tests go in at the same time as the patches.  Can any of this functionality be tested yet?  i.e. are there commands that will exercise this code?  Also still haven't seen tests go in for the basic minidump functionality, like a test that just creates and loads a minidump without any errors.  Do you think it's better to do that now or as a followup patch?  Either way, I don't think we should wait too much longer before building up some tests.


================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp:160
@@ -161,3 +159,3 @@
         const ULONG32 thread_count = thread_list_ptr->NumberOfThreads;
         assert(thread_count < std::numeric_limits<int>::max());
         for (int i = 0; i < thread_count; ++i) {
----------------
Shouldn't this be `std::numeric_limits<uint32_t>::max()` and the loop index variable be unsigned as well?

================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp:173
@@ -174,1 +172,3 @@
 {
+    if (m_data_up && m_data_up->m_exception_sp)
+    {
----------------
Can you invert this conditional and early-out to keep the indentation level reduced?

================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp:320
@@ -312,6 +319,3 @@
+    if (system_info_ptr)
     {
         switch (system_info_ptr->ProcessorArchitecture)
----------------
Same here

================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp:339
@@ +338,3 @@
+    auto exception_stream_ptr = static_cast<MINIDUMP_EXCEPTION_STREAM*>(FindDumpStream(ExceptionStream, &size));
+    if (exception_stream_ptr)
+    {
----------------
And here.


http://reviews.llvm.org/D12126