[Lldb-commits] [PATCH] D63166: Move common functionality from processwindows into processdebugger

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I'm OK with moving common stuff out for now, and I like the separation of ProcessWindows and ProcessDebugger.

I agree that we don't want to live too long in a state with a regular plugin and a remote plugin, I still think there's advantage to having common Windows stuff grouped together (though, perhaps, not exactly this grouping in the long run).  I'm trying to think through the implications on cross-platform post-mortem debugging, e.g., debugging a Windows minidump on a Linux host without spinning up a remote on an actual Windows machine.

This LGTM as an incremental step if you address some of the naming slips and others' feedback.



================
Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.cpp:125
+    StreamString stream;
+    stream.Printf("ProcessWindows unable to launch '%s'.  ProcessWindows can "
+                  "only be used for debug launches.",
----------------
s/ProcessWindows/ProcessDebugger/  (2x)


================
Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.cpp:215
+    // DebuggerThread. StopDebugging() will trigger a call back into
+    // ProcessWindows which will acquire the lock again, so we need to not
+    // deadlock.
----------------
s/ProcessWindows/ProcessDebugger/ ?



================
Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.h:1
+//===-- NativeProcessWindows.h ----------------------------------*- C++ -*-===//
+//
----------------
Please fix file title.


================
Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.h:30
+
+class ProcessWindowsData {
+public:
----------------
Arguably, this should be renamed to ProcessDebuggerData, but that's not a sticking point right now.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63166/new/

https://reviews.llvm.org/D63166