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
Attachment:
pgpg3qwj30Zm8.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel