[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

[Hui Huang via Phabricator via lldb-commits]

Hui added inline comments.


================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:31
+class NativeProcessWindows : public NativeProcessProtocol,
+                             public ProcessDebugger {
+
----------------
labath wrote:
> Hui wrote:
> > labath wrote:
> > > I'm not sure this multiple inheritance is really helping anything here. While looking through the code i kept wondering "why doesn't this function just do the job itself instead of delegating to somebody else" only to have to remind myself that the function it is delegating to is actually a different object, which does not know anything about the bigger picture.
> > > 
> > > If using composition here wouldn't introduce additional complexities/code (which it doesn't look like it would), I think it might be better to do that instead.
> > Do you mean to move what is in ProcessDebugger into nativeprocess instead?  
> No, just instead of inheriting from a ProcessDebugger, making it a member variable instead. Unless there are reasons which make that hard/impossible...
Debug event callback in ProcessDebugger need to be overridden by both processwindows and nativeprocess. Not sure if the change is doable. Also same change is required to processwindows. That will make the patch a little bigger.


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

https://reviews.llvm.org/D63165