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

Sat Mar 2 13:00:51 PST 2019

labath added a comment.

In D58842#1415542 <https://reviews.llvm.org/D58842#1415542>, @zturner wrote:

> 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 agree it's not a top priority, but I think it would be nice to factor that out. The reason for that is that ProcessLaunchInfo is also used to describe remote launches, and in this case, having a Host PTY around doesn't make sense (we might (and we do) want to have a flag that says a pty should be used on the remote side, but an actual host pty object does not make sense there).

> 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.

Ok, I am fine with putting this in Utility then (unless someone else speaks up about his preference for the location of this). In the future, I am starting to think we could have a new package (name TBD) which would contain things that "describe some property of a process, but without any bias towards a particular implementation of the process (Target, Host, lldb-server, ...)". I will soon need to move MemoryRegionInfo out of Utility and into something else, and it seems to me this class could fit that description too.

> 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

Ok so it seems it's not clear if I answered all of Greg's comments or not. Let's wait to see if he has anything to say until monday. If that patch takes longer, then you commit this, and I'll rebase that patch on top of this.

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

https://reviews.llvm.org/D58842