[Spice-devel] [spice-gtk 2/2] Introduce gtask-helper.[ch]

[Christophe Fergeau]

On Wed, Mar 23, 2016 at 10:48:31AM +0100, Fabiano Fidêncio wrote:
> As noticed, GTask's heurestic for return a task in idle or immediately
> doesn't work when using Coroutine

s/heurestic/heuristic/

Imo we need a detailed description of what is not working well with
GTask + coroutine, and one concrete example when the right thing is not
done (either in this log, or as a comment in gtask-helper.[ch]). This
will be really helpful if we ever need to revisit this.


> and that's okay, we just need to do
> the idle ourself. And in order to avoid code duplication, let's
> introduce and make usage of the new g_task_helper_return_* functions.
> 
> These functions match exactly with the existing g_task_return_*
> functions and the only difference is that they return in idle instead of
> returning immediately.
> 
> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> ---
>  m4/manywarnings.m4           |   1 -
>  src/Makefile.am              |   2 +
>  src/channel-base.c           |   4 +-
>  src/channel-main.c           |  12 ++--
>  src/channel-usbredir.c       |  12 ++--
>  src/gtask-helper.c           | 152 +++++++++++++++++++++++++++++++++++++++++++
>  src/gtask-helper.h           |  43 ++++++++++++
>  src/spice-channel.c          |   4 +-
>  src/spice-gstaudio.c         |   6 +-
>  src/usb-acl-helper.c         |  12 ++--
>  src/usb-device-manager.c     |  18 ++---
>  src/vmcstream.c              |  35 ++--------
>  src/win-usb-driver-install.c |  21 +++---
>  13 files changed, 254 insertions(+), 68 deletions(-)
>  create mode 100644 src/gtask-helper.c
>  create mode 100644 src/gtask-helper.h

Do we need to use the helper everywhere, or only in specific cases?

> 
> diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
> index 3e6dd21..ead38a2 100644
> --- a/m4/manywarnings.m4
> +++ b/m4/manywarnings.m4
> @@ -182,7 +182,6 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC],
>      -Wstrict-overflow \
>      -Wstrict-prototypes \
>      -Wsuggest-attribute=const \
> -    -Wsuggest-attribute=format \

Unrelated ?

>      -Wsuggest-attribute=noreturn \
>      -Wsuggest-attribute=pure \
>      -Wswitch \


> diff --git a/src/gtask-helper.h b/src/gtask-helper.h
> new file mode 100644
> index 0000000..9020506
> --- /dev/null
> +++ b/src/gtask-helper.h
> @@ -0,0 +1,43 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2016 Red Hat, Inc.
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.0 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, write to the Free Software
> +   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
> +*/
> +#ifndef __G_TASK_HELPER_H__
> +#define __G_TASK_HELPER_H__
> +
> +#include <gio/gio.h>
> +
> +G_BEGIN_DECLS
> +
> +void g_task_helper_return_boolean(GTask *task,
> +                                  gboolean boolean);

In my opinion, there should be a 'idle' somewhere in the name of all
these helpers. Someone reading unrelated code would have to check what
these helpers are about with the current naming, with an 'idle' in the
name, it's much easier to guess what they are about and decide not to
care about the details.


> +void g_task_helper_return_int(GTask *task,
> +                              gint integer);

g_task_return_int takes a gssize, not a gint.

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