diff mbox series

[v7,1/6] coreaudio: Remove unnecessary explicit casts

Message ID 20250124-coreaudio-v7-1-9d9a4d91db37@daynix.com (mailing list archive)
State New
Headers show
Series coreaudio fixes | expand

Commit Message

Akihiko Odaki Jan. 24, 2025, 5:12 a.m. UTC 
coreaudio had unnecessary explicit casts and they had extra whitespaces
around them so remove them.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 audio/coreaudio.m | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Christian Schoenebeck Jan. 24, 2025, 9:39 a.m. UTC | #1 
On Friday, January 24, 2025 6:12:04 AM CET Akihiko Odaki wrote:
> coreaudio had unnecessary explicit casts and they had extra whitespaces
> around them so remove them.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  audio/coreaudio.m | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/audio/coreaudio.m b/audio/coreaudio.m
> index cadd729d50537850d81718b9284efed5877d9185..0b67347ad7e8c43a77af308a1a3a654dd7084083 100644
> --- a/audio/coreaudio.m
> +++ b/audio/coreaudio.m
> @@ -309,7 +309,7 @@ static OSStatus audioDeviceIOProc(
>      UInt32 frameCount, pending_frames;
>      void *out = outOutputData->mBuffers[0].mData;
>      HWVoiceOut *hw = hwptr;
> -    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hwptr;
> +    coreaudioVoiceOut *core = hwptr;

Well, hwptr is void*, so both versions are fine.

struct name 'coreaudioVoiceOut' should start with upper case BTW.

>      size_t len;
>  
>      if (coreaudio_buf_lock (core, "audioDeviceIOProc")) {
> @@ -392,10 +392,10 @@ static OSStatus init_out_device(coreaudioVoiceOut *core)
>      }
>  
>      if (frameRange.mMinimum > core->frameSizeSetting) {
> -        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum;
> +        core->audioDevicePropertyBufferFrameSize = frameRange.mMinimum;
>          dolog ("warning: Upsizing Buffer Frames to %f\n", frameRange.mMinimum);
>      } else if (frameRange.mMaximum < core->frameSizeSetting) {
> -        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum;
> +        core->audioDevicePropertyBufferFrameSize = frameRange.mMaximum;
>          dolog ("warning: Downsizing Buffer Frames to %f\n", frameRange.mMaximum);
>      } else {
>          core->audioDevicePropertyBufferFrameSize = core->frameSizeSetting;

Those casts are actually necessary, as AudioValueRange's members are Float64
(a.k.a. double) types.

/Christian
Akihiko Odaki Jan. 25, 2025, 5:58 a.m. UTC | #2 
On 2025/01/24 18:39, Christian Schoenebeck wrote:
> On Friday, January 24, 2025 6:12:04 AM CET Akihiko Odaki wrote:
>> coreaudio had unnecessary explicit casts and they had extra whitespaces
>> around them so remove them.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   audio/coreaudio.m | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/audio/coreaudio.m b/audio/coreaudio.m
>> index cadd729d50537850d81718b9284efed5877d9185..0b67347ad7e8c43a77af308a1a3a654dd7084083 100644
>> --- a/audio/coreaudio.m
>> +++ b/audio/coreaudio.m
>> @@ -309,7 +309,7 @@ static OSStatus audioDeviceIOProc(
>>       UInt32 frameCount, pending_frames;
>>       void *out = outOutputData->mBuffers[0].mData;
>>       HWVoiceOut *hw = hwptr;
>> -    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hwptr;
>> +    coreaudioVoiceOut *core = hwptr;
> 
> Well, hwptr is void*, so both versions are fine.
> 
> struct name 'coreaudioVoiceOut' should start with upper case BTW.
> 
>>       size_t len;
>>   
>>       if (coreaudio_buf_lock (core, "audioDeviceIOProc")) {
>> @@ -392,10 +392,10 @@ static OSStatus init_out_device(coreaudioVoiceOut *core)
>>       }
>>   
>>       if (frameRange.mMinimum > core->frameSizeSetting) {
>> -        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum;
>> +        core->audioDevicePropertyBufferFrameSize = frameRange.mMinimum;
>>           dolog ("warning: Upsizing Buffer Frames to %f\n", frameRange.mMinimum);
>>       } else if (frameRange.mMaximum < core->frameSizeSetting) {
>> -        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum;
>> +        core->audioDevicePropertyBufferFrameSize = frameRange.mMaximum;
>>           dolog ("warning: Downsizing Buffer Frames to %f\n", frameRange.mMaximum);
>>       } else {
>>           core->audioDevicePropertyBufferFrameSize = core->frameSizeSetting;
> 
> Those casts are actually necessary, as AudioValueRange's members are Float64
> (a.k.a. double) types.

Explicit casts are unnecessary. Implicit casts still happen at every 
line changed with this patch.

Regards,
Akihiko Odaki
Christian Schoenebeck Jan. 25, 2025, 10:41 a.m. UTC | #3 
On Saturday, January 25, 2025 6:58:30 AM CET Akihiko Odaki wrote:
> On 2025/01/24 18:39, Christian Schoenebeck wrote:
> > On Friday, January 24, 2025 6:12:04 AM CET Akihiko Odaki wrote:
> >> coreaudio had unnecessary explicit casts and they had extra whitespaces
> >> around them so remove them.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   audio/coreaudio.m | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/audio/coreaudio.m b/audio/coreaudio.m
> >> index cadd729d50537850d81718b9284efed5877d9185..0b67347ad7e8c43a77af308a1a3a654dd7084083 100644
> >> --- a/audio/coreaudio.m
> >> +++ b/audio/coreaudio.m
> >> @@ -309,7 +309,7 @@ static OSStatus audioDeviceIOProc(
> >>       UInt32 frameCount, pending_frames;
> >>       void *out = outOutputData->mBuffers[0].mData;
> >>       HWVoiceOut *hw = hwptr;
> >> -    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hwptr;
> >> +    coreaudioVoiceOut *core = hwptr;
> > 
> > Well, hwptr is void*, so both versions are fine.
> > 
> > struct name 'coreaudioVoiceOut' should start with upper case BTW.
> > 
> >>       size_t len;
> >>   
> >>       if (coreaudio_buf_lock (core, "audioDeviceIOProc")) {
> >> @@ -392,10 +392,10 @@ static OSStatus init_out_device(coreaudioVoiceOut *core)
> >>       }
> >>   
> >>       if (frameRange.mMinimum > core->frameSizeSetting) {
> >> -        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum;
> >> +        core->audioDevicePropertyBufferFrameSize = frameRange.mMinimum;
> >>           dolog ("warning: Upsizing Buffer Frames to %f\n", frameRange.mMinimum);
> >>       } else if (frameRange.mMaximum < core->frameSizeSetting) {
> >> -        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum;
> >> +        core->audioDevicePropertyBufferFrameSize = frameRange.mMaximum;
> >>           dolog ("warning: Downsizing Buffer Frames to %f\n", frameRange.mMaximum);
> >>       } else {
> >>           core->audioDevicePropertyBufferFrameSize = core->frameSizeSetting;
> > 
> > Those casts are actually necessary, as AudioValueRange's members are Float64
> > (a.k.a. double) types.
> 
> Explicit casts are unnecessary. Implicit casts still happen at every 
> line changed with this patch.

Wooo, I wasn't aware that QEMU doesn't use -Wconversion. I am not used to 
that. To me it makes sense to warn especially for things like implicit casts
from floating point to integer, as it would be the case here.

/Christian
Akihiko Odaki Jan. 26, 2025, 4 a.m. UTC | #4 
On 2025/01/25 19:41, Christian Schoenebeck wrote:
> On Saturday, January 25, 2025 6:58:30 AM CET Akihiko Odaki wrote:
>> On 2025/01/24 18:39, Christian Schoenebeck wrote:
>>> On Friday, January 24, 2025 6:12:04 AM CET Akihiko Odaki wrote:
>>>> coreaudio had unnecessary explicit casts and they had extra whitespaces
>>>> around them so remove them.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    audio/coreaudio.m | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/audio/coreaudio.m b/audio/coreaudio.m
>>>> index cadd729d50537850d81718b9284efed5877d9185..0b67347ad7e8c43a77af308a1a3a654dd7084083 100644
>>>> --- a/audio/coreaudio.m
>>>> +++ b/audio/coreaudio.m
>>>> @@ -309,7 +309,7 @@ static OSStatus audioDeviceIOProc(
>>>>        UInt32 frameCount, pending_frames;
>>>>        void *out = outOutputData->mBuffers[0].mData;
>>>>        HWVoiceOut *hw = hwptr;
>>>> -    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hwptr;
>>>> +    coreaudioVoiceOut *core = hwptr;
>>>
>>> Well, hwptr is void*, so both versions are fine.
>>>
>>> struct name 'coreaudioVoiceOut' should start with upper case BTW.
>>>
>>>>        size_t len;
>>>>    
>>>>        if (coreaudio_buf_lock (core, "audioDeviceIOProc")) {
>>>> @@ -392,10 +392,10 @@ static OSStatus init_out_device(coreaudioVoiceOut *core)
>>>>        }
>>>>    
>>>>        if (frameRange.mMinimum > core->frameSizeSetting) {
>>>> -        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum;
>>>> +        core->audioDevicePropertyBufferFrameSize = frameRange.mMinimum;
>>>>            dolog ("warning: Upsizing Buffer Frames to %f\n", frameRange.mMinimum);
>>>>        } else if (frameRange.mMaximum < core->frameSizeSetting) {
>>>> -        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum;
>>>> +        core->audioDevicePropertyBufferFrameSize = frameRange.mMaximum;
>>>>            dolog ("warning: Downsizing Buffer Frames to %f\n", frameRange.mMaximum);
>>>>        } else {
>>>>            core->audioDevicePropertyBufferFrameSize = core->frameSizeSetting;
>>>
>>> Those casts are actually necessary, as AudioValueRange's members are Float64
>>> (a.k.a. double) types.
>>
>> Explicit casts are unnecessary. Implicit casts still happen at every
>> line changed with this patch.
> 
> Wooo, I wasn't aware that QEMU doesn't use -Wconversion. I am not used to
> that. To me it makes sense to warn especially for things like implicit casts
> from floating point to integer, as it would be the case here.

I tried '--extra-cflags=-Wconversion -Wno-sign-conversion -Wno-error'. 
It may be actually spotting real bugs, there are just too many warnings.

For this particular case, the implicit casts will never change the 
values because the actual values are integers.

AudioHardware.h says kAudioDevicePropertyBufferFrameSizeRange is "an 
AudioValueRange indicating the minimum and maximum values, inclusive, 
for kAudioDevicePropertyBufferFrameSize." 
kAudioDevicePropertyBufferFrameSize is UInt32 so the values should 
always be in the range of UInt32. The number of frames cannot be a 
fractional value after all. They have Float64 values probably because 
Apple were so lazy that they reused the AudioValueRange type that has 
Float64 members.

Regards,
Akihiko Odaki
diff mbox series

Patch

diff --git a/audio/coreaudio.m b/audio/coreaudio.m
index cadd729d50537850d81718b9284efed5877d9185..0b67347ad7e8c43a77af308a1a3a654dd7084083 100644
--- a/audio/coreaudio.m
+++ b/audio/coreaudio.m
@@ -309,7 +309,7 @@  static OSStatus audioDeviceIOProc(
     UInt32 frameCount, pending_frames;
     void *out = outOutputData->mBuffers[0].mData;
     HWVoiceOut *hw = hwptr;
-    coreaudioVoiceOut *core = (coreaudioVoiceOut *) hwptr;
+    coreaudioVoiceOut *core = hwptr;
     size_t len;
 
     if (coreaudio_buf_lock (core, "audioDeviceIOProc")) {
@@ -392,10 +392,10 @@  static OSStatus init_out_device(coreaudioVoiceOut *core)
     }
 
     if (frameRange.mMinimum > core->frameSizeSetting) {
-        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMinimum;
+        core->audioDevicePropertyBufferFrameSize = frameRange.mMinimum;
         dolog ("warning: Upsizing Buffer Frames to %f\n", frameRange.mMinimum);
     } else if (frameRange.mMaximum < core->frameSizeSetting) {
-        core->audioDevicePropertyBufferFrameSize = (UInt32) frameRange.mMaximum;
+        core->audioDevicePropertyBufferFrameSize = frameRange.mMaximum;
         dolog ("warning: Downsizing Buffer Frames to %f\n", frameRange.mMaximum);
     } else {
         core->audioDevicePropertyBufferFrameSize = core->frameSizeSetting;