[Lldb-commits] [PATCH] D11611: Create a Windows mini-dump target

[Adrian McCarthy]

amccarth marked 6 inline comments as done.
amccarth added a comment.

By the way, I also made the capitalization of WinMiniDump/WinMinidump consistent, going with the former because it's consistent with more Windows APIs (even though the latter is probably more "natural").


================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp:237-238
@@ +236,4 @@
+{
+    // TODO
+    return ArchSpec();
+}
----------------
zturner wrote:
> We already have a `DetermineArchitecture()` function.  Is there any reason we can't just return it right here?  Does this function have slightly different semantics or something?
DetermineArchitecture can be called only after the core file has been opened.  In practice, it seems the GetArchitecture is called at least once before that happens.  In those cases, GetArchitecture returns a default that seems mostly right.

The ELF core handling works similarly, providing a default until the actual value is determined from the core file.

================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.h:86
@@ +85,3 @@
+
+    lldb_private::FileSpec m_core_file;  // TODO:  IS THIS NEEDED?  SHOULD IT BE IN struct Data?
+
----------------
zturner wrote:
> I'd rather remove the TODO and either put it in Data or leave it here.  Either way, be decisive :)
Sorry, I meant to take care of that before sending it out for review, but I searched for `TODO(amccarth)` instead of just TODO.  :-(

Moved to the private ProcessWinMap::Data class.


http://reviews.llvm.org/D11611