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

[Christophe Fergeau]

On Thu, Mar 02, 2017 at 11:46:34AM +0100, Victor Toso wrote:
> Hi,
> 
> Thanks for taking a look :)
> 
> On Thu, Mar 02, 2017 at 10:59:12AM +0100, Christophe Fergeau wrote:
> > On Thu, Mar 02, 2017 at 10:23:45AM +0100, Victor Toso wrote:
> > > Hi,
> > >
> > > On Tue, Feb 28, 2017 at 12:21:45PM +0100, Victor Toso wrote:
> > > > From: Victor Toso <me at victortoso.com>
> > > >
> > > > Hi,
> > > >
> > > > v2->v3:
> > > > * Breaking spice_channel_read_wire() into smaller changes. (teuf)
> > > >
> > > > v2: https://lists.freedesktop.org/archives/spice-devel/2017-February/035455.html
> > > > v1: https://lists.freedesktop.org/archives/spice-devel/2017-February/035446.html
> > > >
> > > > Cheers,
> > > >
> > > > Victor Toso (6):
> > > >   spice-channel: move out non blocking logic of _read_wire()
> > > >   spice-channel: move out non blocking logic of _flush_wire()
> > > >   spice_channel_read_wire: prefer while(TRUE) instead of goto
> > > >   spice_channel_read_wire: move variables to internal scope
> > >
> > > Any feedback on 3/6 and 4/6 ? I don't mind dropping the 5/6 and 6/6
> > > patches at all. They don't bring much benefit and it might be just
> > > personal preference.
> >
> > I was looking at this series yesterday, and I'm not really convinced it
> > brings much, the while (TRUE) which is iterated once at most is as weird
> > as the goto, before the patch the code is
> 
> We should only use goto to deal with errors or exceptions. Not sure why
> here would be an exception.
> 
> https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html

The code is preexisting, it's arguably error handling, and the while
(TRUE) is equally odd if you ask me (mostly because you said it would
loop at most once anyway). I guess I could live with it with a slightly
better changelog, and maybe a comment indicating this is not really a
loop.

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/dbb7ee06/attachment.sig>