Re: [PATCH 10/10] remote: one struct remote_state per struct remote_target

[Pedro Alves <palves at redhat dot com>]

On 05/22/2018 04:37 AM, Simon Marchi wrote:

> I took a rather quick look, because a lot of the changes are mecanical,
> once you have set the premises you pointed out in the commit message
> (and I think they are fine).
> 
> Two nits:
> 
> Is there a reason not to make the remote_state object a simple field
> of remote_target, does it have to be a pointer?  You would have to
> shuffle things around a little bit more, but it seems to work fine.

Yeah, no reason other than struct remote_state not being complete yet 
when the field is defined in struct remote_target.  I was thinking the
moving would be done as follow up, to avoid even more churn mixed in with
changes, very much like patch #4 started with a pointer and then patch #5
moved to objects.

> 
>> @@ -6287,7 +6512,7 @@ remote_target::commit_resume ()
>>       we end up with too many actions for a single packet vcont_builder
>>       flushes the current vCont packet to the remote side and starts a
>>       new one.  */
>> -  struct vcont_builder vcont_builder;
>> +  struct vcont_builder vcont_builder (this);
>>    vcont_builder.restart ();
> 
> That's more a comment for the previous patch, but: I find it strange to
> have to call restart just after building the object. Couldn't the
> constructor leave it in a ready to use state?

Yeah, I guess it could.  I've made that change.  I'll post it
in response to the previous patch.

Thanks,
Pedro Alves