On Mon, 2019-01-28 at 17:50 +0100, Georg Chini wrote:
> > > 2) For read_string() and read_raw() it is difficult to retain a default
> > > value.
> > > The string returned by the functions is a char, but it must not be freed
> > > because it is part of the larger parameter string. So the string passed
> > > as default would need to be a const char, which makes some type casting
> > > necessary. Therefore I would like to keep it as is for the string functions.
> > Is this about not changing the pass-by-reference arguments to functions
> > until success? If so, that seems like a separate issue from whether the
> > argument should be const or not. Just use an internal temporary
> > variable in the functions and only assign to the user variable at the
> > end of the function.
> >
> > Regarding constness, I think the function parameter can be kept non-
> > const even if it's not supposed to be freed (just make sure to document
> > that). It's ok to modify the string contents, nothing will break.
>
> As said on IRC I think you did not get the point. Consider the
> following code:
>
> char *result;
>
> /* To demonstrate the point, I fill result with a newly allocated string */
> result = pa_xstrdup("A string");
>
> /* Now I call read_string() */
> err = pa_message_params_read_string(parameter_list, &result, &state);
>
> Now, if the default value is returned, the string in result must be freed.
> If the value returned is from the parameter list, the string in result must
> NOT be freed. You could work around it by using typecasts or by deciding
> if the string needs to be freed based on the err value, but I think
> this is very ugly and there is nothing that prevents a user doing it the
> wrong way. Therefore I would suggest not to use default values for the
> string functions. For read_raw() a default seems pointless anyway,
> so only the functions read_string() and read_string_array() would be
> affected. For the numeric/boolean functions there is no such issue.
It's an error to pass a string that needs freeing to
pa_message_params_read_string(), just like it's an error to pass a
string that needs freeing to pa_tagstruct_gets().
Your original patch didn't free the parameter either, so I don't know
why you started to worry about this. Maybe you misunderstood this
sentence:
"It should be mentioned that if allocate is true, the result should be
freed with pa_xfree()."
Taken out of context (as was the case when I quoted it), it could be
understood so that pa_message_params_read_string() should free the
function parameter before assigning a new value to it, but I didn't
mean that. I meant that the function documentation should tell the
caller to use pa_xfree() (rather than plain free() or whatever else)
when freeing the returned string.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
