Re: [PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)

[Bruno Jesus]

On Sat, Feb 20, 2016 at 4:18 AM, Charles Davis  wrote:
> Signed-off-by: Charles Davis
>
> 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

[PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)

[Charles Davis]

On Fri, Feb 19, 2016 at 9:49 PM, Bruno Jesus  wrote:
> On Sat, Feb 20, 2016 at 4:18 AM, Charles Davis  wrote:
> > Signed-off-by: Charles Davis
> >
> > 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

[PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)

[Dmitry Timoshkov]

Charles Davis  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?

[PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)

[Ken Thomases]

On Feb 20, 2016, at 12:47 AM, Dmitry Timoshkov  wrote:
> Charles Davis  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

[PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)

[Alexandre Julliard]

Ken Thomases  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.

[PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)

[Andrew Eikum]

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

[PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)

[Ken Thomases]

On Feb 20, 2016, at 8:26 AM, Andrew Eikum  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

[PATCH 2/2] winecoreaudio.drv: Don't print 32-bit values as long integers. (Clang)

[Alexandre Julliard]

Ken Thomases  writes:
> On Feb 20, 2016, at 8:26 AM, Andrew Eikum  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.