[Lldb-commits] [PATCH] D58842: Move ProcessInstanceInfo and similar to Utility

Fri Mar 1 13:34:28 PST 2019

zturner added a comment.

In D58842#1415526 <https://reviews.llvm.org/D58842#1415526>, @labath wrote:

> Hmm.. I think I've thought of (or remembered, because I have been thinking about Utility too) a reason why it might be better to keep these in Host. ProcessLaunchInfo cannot be trivially moved to Utility (and indeed, in this patch, you are keeping it in Host), because it contains some references to Host-specific code (PTY stuff). Now, I am not convinced having the PTY class be a member of ProcessLaunchInfo is a good thing, but assuming we don't want to refactor that now, I think it might be better to keep Process(Launch)Info and friends in the same package (i.e., in Host), at least until we have a reason to do otherwise.

Well, the biggest difference between ProcessAttachInfo / ProcessLaunchInfo and the ones here is that those actually have some non-trivial functionality associated with them.  It might be possible to decouple that functionality from the descriptions themselves, but it didn't seem worth it to me.

I have a mild preference for putting them in Utility on the grounds that logically that's where they seem to make the most sense.  A Process could just as easily be running on a target as on the host, and we may want to pass this description around, and so favoring one or the other regarding to put them seemed a bit biased.  We do already have a process class in Host called HostProcess, and that's more what I envision a host-specific process looking like.  Low level methods that map directly to system calls to manipulate and query a process's state, etc.

As for D58167 <https://reviews.llvm.org/D58167>, I mostly just had a drive-by passing comment, and it looked fine to me after that, but I guess I was waiting for someone else to click Accept since my comment was fairly minor.  But I'll go ahead and accept it anyway just to unblock

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

https://reviews.llvm.org/D58842