[Spice-devel] [PATCH] Use GByteArray to implement RedsClientMonitorsConfig

[Christophe Fergeau]

On Tue, Oct 08, 2013 at 07:03:20AM -0400, Marc-André Lureau wrote:
> 
> ----- Original Message -----
> > reds_on_main_agent_monitor_config() is manually implementing
> > a grow-on-append char *. glib's GByteArray can do this for us,
> > using it makes the code simpler to read.
> 
> Also I don't clearly see the benefit in this case,
> realloc fits pretty well. Overall you saved only 3 lines, and use a more
> complex structure.

The benefit is readability, you don't need to parse the realloc+memcpy+..
and then understand what they do, g_byte_array_append immediatly tells us
what the code intends to do.
With the realloc + memcpy, questions that comes to my mind while reading
the code are if realloc is used properly (no leak in error case), if the
code is really doing an append or doing something subtly different, also,
what about that 'buffer_pos' variable, is it unused, is it useful? I
don't want to have to worry about all of this when looking at how the
monitors config stuff. 

I really don't like high level code (what on_main_agent_monitors_config()
tries to achieve) being mixed with low level bookkeeping stuff.


> There are gazillions of places where GLib structures would help. I don't think we should start rewriting them in master.

I would not advise going over the whole codebase doing such replacements,
but imo we really should try to incrementally improving things when
touching a piece of code where glib would fit. Lack of helper functions,
helper classes, ... combined with big functions/files opencoding all of
this is one of the things making the server code difficult to follow, so we
should improve that little by little ;)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20131008/617ee0ad/attachment.pgp>