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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
