[Spice-devel] [spice-gtk v3 0/6] spice-channel: read/flush wire functions

[Christophe Fergeau]

On Thu, Mar 02, 2017 at 02:05:21PM +0100, Victor Toso wrote:
> > and the while (TRUE) is equally odd if you ask me (mostly because you
> > said it would loop at most once anyway).
> 
> The iteration should happen only once because g_coroutine_socket_wait()
> does wait G_IO_IN/G_IO_OUT. Its very unlikely that after
> g_coroutine_socket_wait() returns, we can't read or write.
> 
> Still, that's not odd. Not odd at all. Unless `goto reread` give us some
> performance gain here, I can't see goto label being preferred to
> while(1).

Yes, in both cases it's not really obvious how many iterations there's
going to be. Just more subtle with the goto than with the while :)

> > I guess I could live with it with a slightly better changelog, and
> > maybe a comment indicating this is not really a loop.
> 
> I can't see a reason to improve the commit log if you don't agree with
> the changes and I don't really intend to keep discussing over this that
> seems more a personal preference thing. I might be completely wrong in
> the end :)

You seem to prefer the while(TRUE), and I don't care that strongly, and
I really think the commit log can be slightly better. "using goto to
reiterate in the code should be avoided." reads as if this was a golden
rule (coming from where?) never ever to be broken. I'd just state that
this makes the code follow the recommendations from SPICE coding style.


> 
> Still, IMO I see an improvement while interacting with this code later
> on. Maybe I should not have sent this to soon but I'm trying to avoid
> huge patch series whenever I can. See this 'wip-qos' patch in current
> master [1] and with this series [2].
> 
> [1] https://gitlab.com/victortoso/spice-gtk/commit/9c68d7875beafbc49df4dc1b8c58a490fbc355b8?view=parallel
> [2] https://gitlab.com/victortoso/spice-gtk/commit/326528c9674dbeb765bb50f6d0b729f8a2b45c57?view=parallel

Thanks, this kind of context can be part of the cover letter.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170302/46515978/attachment.sig>