2008/10/2 Massimo Cora'
<maxcvs email it>
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
