[Spice-devel] [PATCH spice-server 3/6] spicevmc: Avoid computing some variable value if not necessary

[Frediano Ziglio]

> 
> 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 at redhat.com>
> > > > ---
> > > >  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