Re: [PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)
On Sat, Feb 20, 2016 at 4:18 AM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com> --- I couldn't get rid of all the casts. In particular, some of them are of a system-defined structure that we obviously can't change.
dlls/winecoreaudio.drv/mmdevdrv.c | 84 +++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 42 deletions(-)
diff --git a/dlls/winecoreaudio.drv/mmdevdrv.c b/dlls/winecoreaudio.drv/mmdevdrv.c index 60ff3d9..90124bc 100644 --- a/dlls/winecoreaudio.drv/mmdevdrv.c +++ b/dlls/winecoreaudio.drv/mmdevdrv.c @@ -385,10 +385,10 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow, WCHAR ***ids, GUID **guids, UINT *num, UINT *def_index) { UInt32 devsize, size; - AudioDeviceID *devices; + unsigned int *devices; AudioDeviceID default_id; AudioObjectPropertyAddress addr; - OSStatus sc; + int sc; int i, ndevices;
Hi, Charles. I usually don't review patches but it feels weird to me that you are changing the type of the variable because AudioObjectGetPropertyData expects an AudioDeviceID and not an unsigned int. Even if they are the same I believe using the correct type is better. The same for OSStatus which is the return for AudioObjectGetPropertyData. Best wishes, Bruno
On Fri, Feb 19, 2016 at 9:49 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
On Sat, Feb 20, 2016 at 4:18 AM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com> --- I couldn't get rid of all the casts. In particular, some of them are of
a
system-defined structure that we obviously can't change.
dlls/winecoreaudio.drv/mmdevdrv.c | 84 +++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 42 deletions(-)
diff --git a/dlls/winecoreaudio.drv/mmdevdrv.c b/dlls/winecoreaudio.drv/mmdevdrv.c index 60ff3d9..90124bc 100644 --- a/dlls/winecoreaudio.drv/mmdevdrv.c +++ b/dlls/winecoreaudio.drv/mmdevdrv.c @@ -385,10 +385,10 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow, WCHAR ***ids, GUID **guids, UINT *num, UINT *def_index) { UInt32 devsize, size; - AudioDeviceID *devices; + unsigned int *devices; AudioDeviceID default_id; AudioObjectPropertyAddress addr; - OSStatus sc; + int sc; int i, ndevices;
Hi, Charles. I usually don't review patches but it feels weird to me that you are changing the type of the variable because AudioObjectGetPropertyData expects an AudioDeviceID and not an unsigned int. Even if they are the same I believe using the correct type is better. The same for OSStatus which is the return for AudioObjectGetPropertyData. It actually feels weird to me, too. Originally, I cast them in the debug prints, but when I did that in advapi32, AJ wanted me to just change the variable type. So that's what I'm doing now.
Chip
Best wishes, Bruno
Charles Davis <cdavis5x(a)gmail.com> wrote:
--- a/dlls/winecoreaudio.drv/mmdevdrv.c +++ b/dlls/winecoreaudio.drv/mmdevdrv.c @@ -385,10 +385,10 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow, WCHAR ***ids, GUID **guids, UINT *num, UINT *def_index) { UInt32 devsize, size; - AudioDeviceID *devices; + unsigned int *devices; AudioDeviceID default_id; AudioObjectPropertyAddress addr; - OSStatus sc; + int sc; int i, ndevices;
Hi, Charles. I usually don't review patches but it feels weird to me that you are changing the type of the variable because AudioObjectGetPropertyData expects an AudioDeviceID and not an unsigned int. Even if they are the same I believe using the correct type is better. The same for OSStatus which is the return for AudioObjectGetPropertyData. It actually feels weird to me, too. Originally, I cast them in the debug prints, but when I did that in advapi32, AJ wanted me to just change the variable type. So that's what I'm doing now.
What's wrong with using correct printf format specifier instead? -- Dmitry.
On Feb 20, 2016, at 12:47 AM, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
Charles Davis <cdavis5x(a)gmail.com> wrote:
--- a/dlls/winecoreaudio.drv/mmdevdrv.c +++ b/dlls/winecoreaudio.drv/mmdevdrv.c @@ -385,10 +385,10 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow, WCHAR ***ids, GUID **guids, UINT *num, UINT *def_index) { UInt32 devsize, size; - AudioDeviceID *devices; + unsigned int *devices; AudioDeviceID default_id; AudioObjectPropertyAddress addr; - OSStatus sc; + int sc; int i, ndevices;
Hi, Charles. I usually don't review patches but it feels weird to me that you are changing the type of the variable because AudioObjectGetPropertyData expects an AudioDeviceID and not an unsigned int. Even if they are the same I believe using the correct type is better. The same for OSStatus which is the return for AudioObjectGetPropertyData. It actually feels weird to me, too. Originally, I cast them in the debug prints, but when I did that in advapi32, AJ wanted me to just change the variable type. So that's what I'm doing now.
What's wrong with using correct printf format specifier instead?
Unfortunately, for historical compatibility reasons, these types are defined as (unsigned) long for 32-bit but (unsigned) int for 64-bit. So, there's no one correct format specifier. Well, at least not a cross-platform one supported by Wine's logging functions. I'm ambivalent about using int instead of OSStatus, because I think it doesn't make the code significantly more confusing. However, using unsigned int instead of AudioDeviceID seems like it loses important semantic information. It's also likely to backslide as new code is written (assuming others habitually follow the API, as I would). Also, stuff like seen in the hunk above where the type of "devices" is changed but the type of "default_id" is not, just because one is used in logging and the other is not, is bothersome. If a new logging statement is added that starts logging default_id, will we have to change its type? That's no way to live. ;) Alexandre, is this really preferable to casting in the logging statements? -Ken
Ken Thomases <ken(a)codeweavers.com> writes:
Unfortunately, for historical compatibility reasons, these types are defined as (unsigned) long for 32-bit but (unsigned) int for 64-bit. So, there's no one correct format specifier. Well, at least not a cross-platform one supported by Wine's logging functions.
I'm ambivalent about using int instead of OSStatus, because I think it doesn't make the code significantly more confusing. However, using unsigned int instead of AudioDeviceID seems like it loses important semantic information. It's also likely to backslide as new code is written (assuming others habitually follow the API, as I would).
Also, stuff like seen in the hunk above where the type of "devices" is changed but the type of "default_id" is not, just because one is used in logging and the other is not, is bothersome. If a new logging statement is added that starts logging default_id, will we have to change its type? That's no way to live. ;)
Alexandre, is this really preferable to casting in the logging statements?
Not necessarily. It's a trade-off between using the correct type and avoiding casts, so it's acceptable to handle AudioDeviceID differently, if you feel that it's better that way. -- Alexandre Julliard julliard(a)winehq.org
On Sat, Feb 20, 2016 at 01:19:43AM -0600, Ken Thomases wrote:
Unfortunately, for historical compatibility reasons, these types are defined as (unsigned) long for 32-bit but (unsigned) int for 64-bit. So, there's no one correct format specifier. Well, at least not a cross-platform one supported by Wine's logging functions.
I'm ambivalent about using int instead of OSStatus, because I think it doesn't make the code significantly more confusing. However, using unsigned int instead of AudioDeviceID seems like it loses important semantic information. It's also likely to backslide as new code is written (assuming others habitually follow the API, as I would).
Also, stuff like seen in the hunk above where the type of "devices" is changed but the type of "default_id" is not, just because one is used in logging and the other is not, is bothersome. If a new logging statement is added that starts logging default_id, will we have to change its type? That's no way to live. ;)
I agree, I don't think this is a good change. I think having the types match the API has a lot of value, even if it's all the same to the compiler, including OSStatus. Does Apple provide inttypes-style (e.g. PRIu64) macros for these types? Can we use them in Wine? Andrew
On Feb 20, 2016, at 8:26 AM, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
Does Apple provide inttypes-style (e.g. PRIu64) macros for these types?
They do. I've checked back to the 10.5 SDK. I'm not sure they help, though. For example, AudioDeviceID is typedef'd as AudioObjectID, which is in turn typedef'd as UInt32. UInt32 is typedef'd as either unsigned long (in 32-bit) or unsigned int (64-bit). The best fit for that would be PRIu32, but that's "u" for both architectures. That's fine for uint32_t, which is it's purpose, but doesn't help for UInt32. It's the right size in both cases, but doesn't eliminate the warning from using %u with an unsigned long on 32-bit. There's no macro which is %lu for 32-bit and %u for 64-bit, which is what we would need. We could define our own, of course.
Can we use them in Wine?
No idea. This is a file that's only compiled on the Mac, so some of the rules may be relaxed. I'm not sure that such macros are less ugly than casting. I guess that's up to you and Alexandre. -Ken
Ken Thomases <ken(a)codeweavers.com> writes:
On Feb 20, 2016, at 8:26 AM, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
Can we use them in Wine?
No idea. This is a file that's only compiled on the Mac, so some of the rules may be relaxed.
I'm not sure that such macros are less ugly than casting. I guess that's up to you and Alexandre.
I don't think that this type of macros would be an improvement over the casts. For cases where we need to dump a certain type in many different places, a helper function using wine_dbg_sprintf can be defined. -- Alexandre Julliard julliard(a)winehq.org
participants (6)
-
Alexandre Julliard -
Andrew Eikum -
Bruno Jesus -
Charles Davis -
Dmitry Timoshkov -
Ken Thomases