On 20.04.2015 11:33 AM, David Henningsson wrote:
> First, thanks for looking after OSX :-)
All for very egoistical reasons. ;)
>
> And then - could you try sending your patch with git send-email (or
> git format-patch and then attach the resulting file)? This looks
> weird, with half of the patch in an attachment and the header outside
> the attachment.
I am using git format-patch. I'm also using --attach. My mail client has
a habit of breaking long lines at a 80 character limit, so this could
potentially destroy the patch/make it unable to apply cleanly, which is
putting an even greater burden on the person who has to apply it in the end.
It probably has to do with Enigmail and and I could work around it by
not signing my mails or hoping that PGP/MIME (which I use anyway) does
not show this weird behavior, but I prefer to sign my mails still.
> On 2015-04-19 08:57, Mihai Moldovan wrote:
>>
>> diff --git a/src/modules/macosx/module-coreaudio-device.c
>> b/src/modules/macosx/module-coreaudio-device.c
>> index cb62661..ad5c07b 100644
>> --- a/src/modules/macosx/module-coreaudio-device.c
>> +++ b/src/modules/macosx/module-coreaudio-device.c
>> @@ -375,6 +375,39 @@ static int ca_sink_set_state(pa_sink *s,
>> pa_sink_state_t state) {
>> return 0;
>> }
>>
>> +/* Caveat: this function frees the CFString if conversion succeeded. */
>
> I'm not actually following the reasoning. Is the CFString leaked if
> the conversion fails, that can't be a good thing?
It means the caller is responsible for freeing it in case conversion
failed. Maybe the caller want to do something to rectify the situation
or use it in some way or another after a failing conversion -- we don't
know and it wouldn't be a good idea to just free it so it'll be unavailable.
If however conversion succeeds, there's no need keeping it around.
>> +static int CFString_to_cstr_n(CFStringRef cfstr, char *buf, long n) {
>
> Maybe return a bool here instead of int? Often an int result of 0
> means success/no error, here's it's the opposite, which is confusing.
Often, but not always... and bool is equivalent to an integer and only
available in C99+. But as the PA code uses it in other places, I guess I
will just change it. (Including using the true and false macros.)
>> + int ret;
>> +
>> + assert (buf);
>> +
>> + ret = 0;
>> +
>> + if (cfstr != NULL) {
>> + const char *tmp = CFStringGetCStringPtr(cfstr,
>> kCFStringEncodingUTF8);
>> +
>> + if (tmp == NULL) {
>> + if (CFStringGetCString(cfstr, buf, n,
>> kCFStringEncodingUTF8))
>> + ret = 1;
>> + }
>> + else {
>> + strncpy(buf, tmp, n);
>> + buf[n - 1] = 0;
>> + ret = 1;
>> + }
>> + }
>> +
>> + /*
>> + * A true value for ret implies cfstr != NULL, but let's still
>> do the check
>> + * for safety reasons (i.e., should this code ever be
>> re-organized...)
>> + */
>> + if (ret && cfstr != NULL) {
>> + CFRelease(cfstr);
>> + }
>
> Can't you just move it a line above, inside the "if (cfstr != NULL)"
> part? Then you don't need to check for it again.
Yes, could do this. I haven't done so for a specific reason, though:
CFRelease() MUST NOT be called on null pointers. It is an error to do so
and the application will crash. This contradicts the normal operation of
free(). This should be very clear, especially when rebasing code.
Thank you for your input! I'll change the patch after your reply, if you
see it still necessary. Will probably do the bool stuff anyway and free
the CFString on error outside of CFString_to_cstr_n(), which I'm
currently not doing (my mistake.)
Mihai
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20150420/41e2be8d/attachment.sig>
