[Qemu-devel] [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[Qemu-devel] [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[Andrzej Zaborowski]

Revision: 4363
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4363
Author:   balrog
Date:     2008-05-06 15:01:12 +0000 (Tue, 06 May 2008)

Log Message:
-----------
MusicPal: fix gcc4 build (Jan Kiszka).

Modified Paths:
--------------
    trunk/hw/musicpal.c

Modified: trunk/hw/musicpal.c
===================================================================
--- trunk/hw/musicpal.c	2008-05-06 14:58:23 UTC (rev 4362)
+++ trunk/hw/musicpal.c	2008-05-06 15:01:12 UTC (rev 4363)
@@ -255,7 +255,8 @@
 static void audio_callback(void *opaque, int free_out, int free_in)
 {
     musicpal_audio_state *s = opaque;
-    int16_t *codec_buffer, *mem_buffer;
+    int16_t *codec_buffer;
+    void *mem_buffer;
     int pos, block_size;
 
     if (!(s->playback_mode & MP_AUDIO_PLAYBACK_EN))
@@ -276,8 +277,9 @@
         if (s->playback_mode & MP_AUDIO_MONO) {
             codec_buffer = wm8750_dac_buffer(s->wm, block_size >> 1);
             for (pos = 0; pos < block_size; pos += 2) {
-                *codec_buffer++ = *mem_buffer;
-                *codec_buffer++ = *mem_buffer++;
+                *codec_buffer++ = *(uint16_t *)mem_buffer;
+                *codec_buffer++ = *(uint16_t *)mem_buffer;
+                mem_buffer += 2;
             }
         } else
             memcpy(wm8750_dac_buffer(s->wm, block_size >> 2),
@@ -286,14 +288,14 @@
         if (s->playback_mode & MP_AUDIO_MONO) {
             codec_buffer = wm8750_dac_buffer(s->wm, block_size);
             for (pos = 0; pos < block_size; pos++) {
-                *codec_buffer++ = cpu_to_le16(256 * *((int8_t *)mem_buffer));
-                *codec_buffer++ = cpu_to_le16(256 * *((int8_t *)mem_buffer)++);
+                *codec_buffer++ = cpu_to_le16(256 * *(int8_t *)mem_buffer);
+                *codec_buffer++ = cpu_to_le16(256 * *(int8_t *)mem_buffer++);
             }
         } else {
             codec_buffer = wm8750_dac_buffer(s->wm, block_size >> 1);
             for (pos = 0; pos < block_size; pos += 2) {
-                *codec_buffer++ = cpu_to_le16(256 * *((int8_t *)mem_buffer)++);
-                *codec_buffer++ = cpu_to_le16(256 * *((int8_t *)mem_buffer)++);
+                *codec_buffer++ = cpu_to_le16(256 * *(int8_t *)mem_buffer++);
+                *codec_buffer++ = cpu_to_le16(256 * *(int8_t *)mem_buffer++);
             }
         }
     }

Re: [Qemu-devel] [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[Paul Brook]

> -    int16_t *codec_buffer, *mem_buffer;
> +    int16_t *codec_buffer;
> +    void *mem_buffer;
>...
> +                mem_buffer += 2;

I really dislike doing arithmetic on a void*.

Paul

[Qemu-devel] Re: [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[Jan Kiszka]

Paul Brook wrote:
>> -    int16_t *codec_buffer, *mem_buffer;
>> +    int16_t *codec_buffer;
>> +    void *mem_buffer;
>> ...
>> +                mem_buffer += 2;
> 
> I really dislike doing arithmetic on a void*.

Only fair. Version below should be better (in several ways).

Index: qemu/hw/musicpal.c
===================================================================
--- qemu/hw/musicpal.c	(Revision 4365)
+++ qemu/hw/musicpal.c	(Arbeitskopie)
@@ -256,7 +256,7 @@ static void audio_callback(void *opaque,
 {
     musicpal_audio_state *s = opaque;
     int16_t *codec_buffer;
-    void *mem_buffer;
+    int8_t *mem_buffer;
     int pos, block_size;
 
     if (!(s->playback_mode & MP_AUDIO_PLAYBACK_EN))
@@ -277,8 +277,8 @@ static void audio_callback(void *opaque,
         if (s->playback_mode & MP_AUDIO_MONO) {
             codec_buffer = wm8750_dac_buffer(s->wm, block_size >> 1);
             for (pos = 0; pos < block_size; pos += 2) {
-                *codec_buffer++ = *(uint16_t *)mem_buffer;
-                *codec_buffer++ = *(uint16_t *)mem_buffer;
+                *codec_buffer++ = *(int16_t *)mem_buffer;
+                *codec_buffer++ = *(int16_t *)mem_buffer;
                 mem_buffer += 2;
             }
         } else
@@ -288,14 +288,14 @@ static void audio_callback(void *opaque,
         if (s->playback_mode & MP_AUDIO_MONO) {
             codec_buffer = wm8750_dac_buffer(s->wm, block_size);
             for (pos = 0; pos < block_size; pos++) {
-                *codec_buffer++ = cpu_to_le16(256 * *(int8_t *)mem_buffer);
-                *codec_buffer++ = cpu_to_le16(256 * *(int8_t *)mem_buffer++);
+                *codec_buffer++ = cpu_to_le16(256 * *mem_buffer);
+                *codec_buffer++ = cpu_to_le16(256 * *mem_buffer++);
             }
         } else {
             codec_buffer = wm8750_dac_buffer(s->wm, block_size >> 1);
             for (pos = 0; pos < block_size; pos += 2) {
-                *codec_buffer++ = cpu_to_le16(256 * *(int8_t *)mem_buffer++);
-                *codec_buffer++ = cpu_to_le16(256 * *(int8_t *)mem_buffer++);
+                *codec_buffer++ = cpu_to_le16(256 * *mem_buffer++);
+                *codec_buffer++ = cpu_to_le16(256 * *mem_buffer++);
             }
         }
     }

Re: [Qemu-devel] Re: [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[andrzej zaborowski]

On 06/05/2008, Jan Kiszka <jan.kiszka@web.de> wrote:
> Paul Brook wrote:
>  >> -    int16_t *codec_buffer, *mem_buffer;
>  >> +    int16_t *codec_buffer;
>  >> +    void *mem_buffer;
>  >> ...
>  >> +                mem_buffer += 2;
>  >
>  > I really dislike doing arithmetic on a void*.
>
>
> Only fair. Version below should be better (in several ways).
>
>  Index: qemu/hw/musicpal.c
>  ===================================================================
>  --- qemu/hw/musicpal.c  (Revision 4365)
>  +++ qemu/hw/musicpal.c  (Arbeitskopie)
>  @@ -256,7 +256,7 @@ static void audio_callback(void *opaque,
>
>  {
>      musicpal_audio_state *s = opaque;
>
>      int16_t *codec_buffer;
>  -    void *mem_buffer;
>
> +    int8_t *mem_buffer;

Well, s->targer_buffer is already void * and you have

    mem_buffer = s->target_buffer + s->play_pos;

I'm not sure what's wrong with void * arithmetics though. They are
specified (somewhere) to work as if the type was a char *, and the
only annoyance is that you have to remember that.

Cheers

Re: [Qemu-devel] Re: [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[Paul Brook]

> I'm not sure what's wrong with void * arithmetics though. They are
> specified (somewhere) to work as if the type was a char *, and the
> only annoyance is that you have to remember that.

It doesn't work in C++ for a start.
I thought it was a GCC extension, though I could be wrong about that.

Paul

Re: [Qemu-devel] Re: [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[Daniel P. Berrange]

On Wed, May 07, 2008 at 12:53:42PM +0100, Paul Brook wrote:
> > I'm not sure what's wrong with void * arithmetics though. They are
> > specified (somewhere) to work as if the type was a char *, and the
> > only annoyance is that you have to remember that.
> 
> It doesn't work in C++ for a start.
> I thought it was a GCC extension, though I could be wrong about that.

IIRC, arithmetic on void * is said to be undefined by the standard - GCC 
treats them the same as char * as a 'convenience' for people...

`-Wpointer-arith'
     Warn about anything that depends on the "size of" a function type
     or of `void'.  GNU C assigns these types a size of 1, for
     convenience in calculations with `void *' pointers and pointers to
     functions.

So I'd say better off just using char * so the behaviour is guarenteed
to be defined & consistent across compilers.

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] Re: [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[Andreas Schwab]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Wed, May 07, 2008 at 12:53:42PM +0100, Paul Brook wrote:
>> > I'm not sure what's wrong with void * arithmetics though. They are
>> > specified (somewhere) to work as if the type was a char *, and the
>> > only annoyance is that you have to remember that.
>> 
>> It doesn't work in C++ for a start.
>> I thought it was a GCC extension, though I could be wrong about that.
>
> IIRC, arithmetic on void * is said to be undefined by the standard

Not only undefined, but a constraint violation, ie. completely outside
the language.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [Qemu-devel] Re: [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[Andreas Schwab]

Paul Brook <paul@codesourcery.com> writes:

> I thought it was a GCC extension,

It is.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [Qemu-devel] Re: [4363] MusicPal: fix gcc4 build (Jan Kiszka).

[andrzej zaborowski]

On 07/05/2008, Paul Brook <paul@codesourcery.com> wrote:
> > I'm not sure what's wrong with void * arithmetics though. They are
>  > specified (somewhere) to work as if the type was a char *, and the
>  > only annoyance is that you have to remember that.
>
>
> It doesn't work in C++ for a start.
>  I thought it was a GCC extension, though I could be wrong about that.

Sorry, you're right.  The specs say that a void pointer should have
the representation and alignment requirements of a char pointer and I
interpreted it wrongly.
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.