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

Pedro Alves
2018-05-22T17:31:05Z

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