>
> On Thu, Jan 26, 2017 at 09:37:56AM -0500, Frediano Ziglio wrote:
> > >
> > > On Thu, Jan 26, 2017 at 10:56:45AM +0000, Frediano Ziglio wrote:
> > > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > > ---
> > > > server/spicevmc.c | 10 +++++-----
> > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > > > index a92ec7f..23e60b3 100644
> > > > --- a/server/spicevmc.c
> > > > +++ b/server/spicevmc.c
> > > > @@ -585,12 +585,13 @@ static uint8_t
> > > > *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> > > > uint32_t size)
> > > > {
> > > > RedVmcChannel *channel;
> > > > - RedClient *client = red_channel_client_get_client(rcc);
> > > > -
> > > > - channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > > + RedClient *client;
> > > >
> > > > switch (type) {
> > > > case SPICE_MSGC_SPICEVMC_DATA:
> > > > + client = red_channel_client_get_client(rcc);
> > > > + channel =
> > > > RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > > +
> > > > assert(!channel->recv_from_client_buf);
> > > >
> > > > channel->recv_from_client_buf =
> > > > red_char_device_write_buffer_get(channel->chardev,
> > > > @@ -615,10 +616,9 @@ static void
> > > > spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> > > > {
> > > > RedVmcChannel *channel;
> > > >
> > > > - channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > > -
> > > > switch (type) {
> > > > case SPICE_MSGC_SPICEVMC_DATA:
> > > > + channel =
> > > > RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > > /* buffer wasn't pushed to device */
> > > > red_char_device_write_buffer_release(channel->chardev,
> > > > &channel->recv_from_client_buf);
> > > > break;
> > >
> > > This is something that I'd just let the compiler handle if needed. Note
> > > that this also change some variables from being always initialized to
> > > sometimes being initialized, which is in my opinion is not necessarily a
> > > good move.
> > >
> > > Christophe
> > >
> >
> > In this case the compiler cannot do it. How can it know that not calling
> > a function does not make any difference?
>
> So red_channel_client_get_channel() should be annotated with
> G_GNUC_PURE so that the compiler knows about this and can optimize
> everywhere by itself?
Not enough and potentially false.
> Thinking more about this patch, even though I hate the {} you have to
> add, I'd be ok with
>
> spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> {
> switch (type) {
> case SPICE_MSGC_SPICEVMC_DATA: {
> RedVmcChannel *channel;
>
> channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> /* buffer wasn't pushed to device */
> red_char_device_write_buffer_release(channel->chardev,
> &channel->recv_from_client_buf);
> break;
> }
>
> which improves the reading of the code in my opinion (you know 'channel'
> is not going to be used anywhere else).
>
> Christophe
>
I was thinking the same, even better using some more C99 style like
switch (type) {
case SPICE_MSGC_SPICEVMC_DATA: {
RedVmcChannel *channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
/* buffer wasn't pushed to device */
red_char_device_write_buffer_release(channel->chardev,
&channel->recv_from_client_buf);
break;
}
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel