On 28.01.19 18:37, Georg Chini wrote:
On 28.01.19 18:05, Tanu Kaskinen wrote:
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.
OK, I was worrying because unlike pa_tagstruct_gets() the
function parameter is currently char ** and not const char **
But after taking a second look, I see that I can make it const
now, so problem solved. Are you OK with leaving read_raw()
without default? Here the parameter must remain char**
because the returned string may be a list that can be
processed again by the parsing functions (and in my opinion
a default in this case does not make sense anyway).
And one more point I stumbled over: When reading numeric
arrays, how should an empty element ({}) be treated? Should
it be an error or treated as 0? My current approach is to read
it as 0.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
