[Spice-devel] [PATCH 3.10/12] Remove a couple single-use static functions

[Frediano Ziglio]

> 
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> red_cursor_marshall_inval(), red_migrate_cursor() and
> on_new_cursor_channel() were short functions that were each only called
> from a single location, so there's no need for them to be separate
> functions.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>

Acked

OT: I don't fully agree with the reasoning. Today compilers are really
good inlining single used static functions. Putting even some lines in
small functions can improve readability if function name is well choosed,
future maintenance and type safety.

Frediano


> ---
>  server/cursor-channel.c |  9 +--------
>  server/red_worker.c     | 40 ++++++++++++++--------------------------
>  2 files changed, 15 insertions(+), 34 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 4811b79..5222bd8 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -294,13 +294,6 @@ static inline void red_marshall_inval(RedChannelClient
> *rcc,
>      spice_marshall_msg_cursor_inval_one(base_marshaller, &inval_one);
>  }
>  
> -static void red_cursor_marshall_inval(RedChannelClient *rcc,
> -                SpiceMarshaller *m, CacheItem *cach_item)
> -{
> -    spice_assert(rcc);
> -    red_marshall_inval(rcc, m, cach_item);
> -}
> -
>  static void cursor_channel_send_item(RedChannelClient *rcc, PipeItem
>  *pipe_item)
>  {
>      SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> @@ -311,7 +304,7 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, PipeItem *pipe_item)
>          cursor_marshall(rcc, m, SPICE_CONTAINEROF(pipe_item, CursorPipeItem,
>          base));
>          break;
>      case PIPE_ITEM_TYPE_INVAL_ONE:
> -        red_cursor_marshall_inval(rcc, m, (CacheItem *)pipe_item);
> +        red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_VERB:
>          red_marshall_verb(rcc, (VerbItem*)pipe_item);
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 2428daf..2967c91 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9901,29 +9901,6 @@ static void handle_new_display_channel(RedWorker
> *worker, RedClient *client, Red
>      on_new_display_channel_client(dcc);
>  }
>  
> -static void red_migrate_cursor(RedWorker *worker, RedChannelClient *rcc)
> -{
> -    if (red_channel_client_is_connected(rcc)) {
> -        red_channel_client_pipe_add_type(rcc,
> -                                         PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> -        red_channel_client_default_migrate(rcc);
> -    }
> -}
> -
> -static void on_new_cursor_channel(RedWorker *worker, RedChannelClient *rcc)
> -{
> -    CursorChannel *channel = worker->cursor_channel;
> -
> -    spice_assert(channel);
> -    red_channel_client_ack_zero_messages_window(rcc);
> -    red_channel_client_push_set_ack(rcc);
> -    // TODO: why do we check for context.canvas? defer this to after display
> cc is connected
> -    // and test it's canvas? this is just a test to see if there is an
> active renderer?
> -    if (worker->surfaces[0].context.canvas &&
> !channel->common.during_target_migrate) {
> -        red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_CURSOR_INIT);
> -    }
> -}
> -
>  static void red_connect_cursor(RedWorker *worker, RedClient *client,
>  RedsStream *stream,
>                                 int migrate,
>                                 uint32_t *common_caps, int num_common_caps,
> @@ -9949,7 +9926,15 @@ static void red_connect_cursor(RedWorker *worker,
> RedClient *client, RedsStream
>      channel->stat = stat_add_node(worker->stat, "cursor_channel", TRUE);
>      channel->common.base.out_bytes_counter = stat_add_counter(channel->stat,
>      "out_bytes", TRUE);
>  #endif
> -    on_new_cursor_channel(worker, &ccc->common.base);
> +
> +    RedChannelClient *rcc = &ccc->common.base;
> +    red_channel_client_ack_zero_messages_window(rcc);
> +    red_channel_client_push_set_ack(rcc);
> +    // TODO: why do we check for context.canvas? defer this to after display
> cc is connected
> +    // and test it's canvas? this is just a test to see if there is an
> active renderer?
> +    if (worker->surfaces[0].context.canvas &&
> !channel->common.during_target_migrate) {
> +        red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_CURSOR_INIT);
> +    }
>  }
>  
>  static void surface_dirty_region_to_rects(RedSurface *surface,
> @@ -10674,12 +10659,15 @@ void handle_dev_cursor_disconnect(void *opaque,
> void *payload)
>  void handle_dev_cursor_migrate(void *opaque, void *payload)
>  {
>      RedWorkerMessageCursorMigrate *msg = payload;
> -    RedWorker *worker = opaque;
>      RedChannelClient *rcc = msg->rcc;
>  
>      spice_info("migrate cursor client");
>      spice_assert(rcc);
> -    red_migrate_cursor(worker, rcc);
> +    if (!red_channel_client_is_connected(rcc))
> +        return;
> +
> +    red_channel_client_pipe_add_type(rcc,
> PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> +    red_channel_client_default_migrate(rcc);
>  }
>  
>  void handle_dev_set_compression(void *opaque, void *payload)
> --
> 2.4.3