On 31.01.19 19:33, Tanu Kaskinen wrote:
On Mon, 2019-01-28 at 18:43 +0100, Georg Chini wrote:
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).
By "leaving without default", do you mean that you want to assign to
the result variable early, when the function might still fail? I don't
see why you'd want to do that, except maybe for avoiding one local
variable.
It's just a general pattern to not touch return variables on failure.
It probably won't cause any trouble in this case if you go against that
pattern, because the caller is unlikely to assign a default value that
would get used in case the function fails, but I really don't see any
good reason to deliberately go against the generally-useful pattern.
Whether the string is const or not doesn't matter here at all, as far
as I can see.
I want to return NULL in case that the function fails.
First, the function read_raw() differs from all other read
functions in so far that the result of the function is expected
to be a string that must be further processed with the other
read functions. So a default does not make any sense and
returning NULL when nothing is read seems sensible.
Second I still think it matters that the function argument is
char and not const char. For the caller, it means that it has
to provide a char that needs not be freed if a default is
wanted. This is ugly and very likely to cause memory leaks.
I do not think we need to implement something just to
follow a pattern if it is no use at all and even likely to cause
errors, but if you insist, I will change it in the next version.
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.
I think it should be an error. When reading single numeric values, we
distinguish between 0 and null, and I don't think we should change the
rules when reading numeric values in arrays.
OK, will change it in the next version.
Meanwhile I created a merge request for the patch set. Please
take a look at it. Patches 2 - 5 are (apart from one single wording
change you requested) unchanged, so I guess they do not need a
thorough review again.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
