2008/10/2 Massimo Cora' <[EMAIL PROTECTED]> > Hi Vivien, > > Vivien Malerba wrote: > > > > I've applied the patch because it does still pass the checks, but I've > > got a few remarks which, I'm sure you can correct quickly ;) > > * once the is_freeable flag is set to FALSE, there is no way it can be > > back to TRUE > > it can be reset to TRUE if a user calls gda_holder_take_value (). > > > * the real_gda_holder_set_const_value() function seems to return NULL > > all the time (the inline doc is not helpfull) > > ok I slightly modified the doc and added a gboolean value_changed so > that we always return the previous stored value, or NULL in case of error. >
I've just committed your patch! However, I still have some small issues: * At line 1022, the value has not changed, and the debug message says that the function returns NULL when in fact it returns holder->priv->value. * whenever NULL is retuned and if it's an error, then g_set_error must be set. > > > * I'm not sure the copy function is correct because it both copies the > > is_freeable flag and the priv->value > > > > well, say for instance that we create a GValue with a static string. We > should copy it with the is_freeable flag set to false, or we may have > some problems trying to free its value. > I see we don't use it into gda-holder.c or within the *take_value > functions. Probably we should warn user of this and avoid to copy the > is_freeable? Yes, printing a warning if some mem leak is about to occur is a good idea. Thanks, Vivien
_______________________________________________ gnome-db-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/gnome-db-list
