Patchwork [02/22] eepro100: cast a void * makes no sense

login
register
mail settings
Submitter Juan Quintela
Date Aug. 24, 2009, 11:03 a.m.
Message ID <51486eb6860d1680c1bce45e310dcd3aae096f43.1251111439.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/31899/
State Superseded
Headers show

Comments

Juan Quintela - Aug. 24, 2009, 11:03 a.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/eepro100.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Stefan Weil - Aug. 24, 2009, 12:56 p.m.
Juan Quintela schrieb:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/eepro100.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 0031d36..09083c2 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>
>  static void nic_reset(void *opaque)
>  {
> -    EEPRO100State *s = (EEPRO100State *) opaque;
> +    EEPRO100State *s = opaque;
>      logout("%p\n", s);
>      static int first;
>      if (!first) {
> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
>
>  static int nic_load(QEMUFile * f, void *opaque, int version_id)
>  {
> -    EEPRO100State *s = (EEPRO100State *) opaque;
> +    EEPRO100State *s = opaque;
>      int i;
>      int ret;
>
> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)
>
>  static void nic_save(QEMUFile * f, void *opaque)
>  {
> -    EEPRO100State *s = (EEPRO100State *) opaque;
> +    EEPRO100State *s = opaque;
>      int i;
>
>      if (s->pci_dev)
>   

I wrote these type casts, and I think they make sense.
In C++ code, they are even mandatory.

I think the arguments why C++ requires this kind of
type casts apply to C code (like in Qemu) as well.

If it is possible with no or very litte efforts to write
code which is C and C++ compatible, I prefer to do so.

So please don't apply this patch.

Regards
Stefan Weil
Juan Quintela - Aug. 24, 2009, 1:04 p.m.
Stefan Weil <Stefan.Weil@weilnetz.de> wrote:
> Juan Quintela schrieb:
>>
>>  static void nic_reset(void *opaque)
>>  {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;
>>      logout("%p\n", s);
>>      static int first;
>>      if (!first) {

Hi

> I wrote these type casts, and I think they make sense.

In C, they made no sense. they are a nop.
In qemu, they are not used almost anywhere.

> In C++ code, they are even mandatory.

This is no C++.


> I think the arguments why C++ requires this kind of
> type casts apply to C code (like in Qemu) as well.

And they are?  I don't see what they buy us.


> If it is possible with no or very litte efforts to write
> code which is C and C++ compatible, I prefer to do so.

If it is possible to have a consistent code base, with little effort,
I prefer to do so.

> So please don't apply this patch.

I think it should be applied (qemu sense), but you do eepro100 work, and
I don't do/plan to do more eepro100 work.

I guess it is your call.

Later, Juan.
Markus Armbruster - Aug. 24, 2009, 1:51 p.m.
Stefan Weil <Stefan.Weil@weilnetz.de> writes:

> Juan Quintela schrieb:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/eepro100.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 0031d36..09083c2 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>>
>>  static void nic_reset(void *opaque)
>>  {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;
>>      logout("%p\n", s);
>>      static int first;
>>      if (!first) {
>> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
>>
>>  static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>  {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;
>>      int i;
>>      int ret;
>>
>> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>
>>  static void nic_save(QEMUFile * f, void *opaque)
>>  {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;
>>      int i;
>>
>>      if (s->pci_dev)
>>   
>
> I wrote these type casts, and I think they make sense.
> In C++ code, they are even mandatory.

Yes, but this isn't C++.

> I think the arguments why C++ requires this kind of
> type casts apply to C code (like in Qemu) as well.
>
> If it is possible with no or very litte efforts to write
> code which is C and C++ compatible, I prefer to do so.

I respectfully disagree.  Casts from "void *" to "T *" are pure noise.
Getting into the habit of writing noise casts runs the risk of silencing
warnings that flag real type errors.
Anthony Liguori - Aug. 24, 2009, 1:51 p.m.
Juan Quintela wrote:
> I think it should be applied (qemu sense), but you do eepro100 work, and
> I don't do/plan to do more eepro100 work.
>   

I agree with Juan.

Overall consistency in the code base is extremely important.  Stefan, if 
you disagree with this style, you should submit a cleanup patch that 
converts every occurrence of this idiom.

Regards,

Anthony Liguori
Gerd Hoffmann - Aug. 24, 2009, 2:05 p.m.
>>   static void nic_save(QEMUFile * f, void *opaque)
>>   {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;

> I wrote these type casts, and I think they make sense.

Why?

There is no technical reason for it.

And in that case it IMHO doesn't make sense to keep the cast as 
documentation.

> If it is possible with no or very litte efforts to write
> code which is C and C++ compatible, I prefer to do so.

Point being?  I doubt qemu will go C++ ...

cheers,
   Gerd
Stefan Weil - Aug. 26, 2009, 1:52 p.m.
Markus Armbruster schrieb:
> Stefan Weil <Stefan.Weil@weilnetz.de> writes:
>
>> Juan Quintela schrieb:
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>> hw/eepro100.c | 6 +++---
>>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>> index 0031d36..09083c2 100644
>>> --- a/hw/eepro100.c
>>> +++ b/hw/eepro100.c
>>> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>>>
>>> static void nic_reset(void *opaque)
>>> {
>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>> + EEPRO100State *s = opaque;
>>> logout("%p\n", s);
>>> static int first;
>>> if (!first) {
>>> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState
>>> *vc, const uint8_t * buf, size_t size
>>>
>>> static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>> {
>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>> + EEPRO100State *s = opaque;
>>> int i;
>>> int ret;
>>>
>>> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void
>>> *opaque, int version_id)
>>>
>>> static void nic_save(QEMUFile * f, void *opaque)
>>> {
>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>> + EEPRO100State *s = opaque;
>>> int i;
>>>
>>> if (s->pci_dev)
>>>
>> I wrote these type casts, and I think they make sense.
>> In C++ code, they are even mandatory.
>
> Yes, but this isn't C++.
>
>> I think the arguments why C++ requires this kind of
>> type casts apply to C code (like in Qemu) as well.
>>
>> If it is possible with no or very litte efforts to write
>> code which is C and C++ compatible, I prefer to do so.
>
> I respectfully disagree. Casts from "void *" to "T *" are pure noise.
> Getting into the habit of writing noise casts runs the risk of silencing
> warnings that flag real type errors.

Hello

Do you only disagree with my first sentence or with both sentences?

Currently, I seem to be alone with my opinion (at least in qemu-devel).
Nevertheless, the designers of C++ thought that casts from void * to
T * were something very important. I don't know the history of their
decision. I personally think that deriving a data type T from some
bytes in memory which can contain anything is an operation which is
worth being documented by the programmer, and this is exactly what
the cast does.

My main reason why I try to write portable code is my personal
experience with code reuse and the future development of compilers.
It is quite possible that some day C compilers will require
type casts like C++ compilers do today.

And even today I want to be able to share C code from QEMU with
program code written in C++ (which makes a large part of my
business work).

Anthony, there is already a mixture of coding styles in QEMU
(also regarding type casts). This is not surprising for an
open source project with many contributors. Do we really
need additional regulations? I think the existing ones
(those in CODING_STYLE) are very good, and they are sufficient.
I'd appreciate it very much if all code in QEMU would
respect this documented coding style. Today, it does not,
and there was an agreement that we do not write commits which
only change the coding style (at least for white space).
I suggest to stick to this agreement for non white space
coding style, too.

Let me give one more C/C++ example. Today, many data structures
are declared like this: typedef struct T { ... } T;
There is nothing wrong with it, but it can be improved in
several ways:

* The declaration does not work with C++ (yes, I know that many
  programmers are not interested in C++ compatibility for QEMU).

* The declaration allows variable declarations using struct T var;
  or just T var; (which is the QEMU style). I think a declaration
  which does not enforce the correct style is less good.

* The declaration contains noise (bad argument, but many people
  like speaking of noise) :-)

Why don't we declare structures like this: typedef struct { ... } T;?
I suggest this to be the new coding style for structure declarations
because it is shorter, C++ compatible and unambiguous.

Kind regards

Stefan
Gerd Hoffmann - Aug. 26, 2009, 2:49 p.m.
Hi,

> Why don't we declare structures like this: typedef struct { ... } T;?
> I suggest this to be the new coding style for structure declarations
> because it is shorter, C++ compatible and unambiguous.

There are quite a few cases where this will simply not work.  They 
usually use a slightly different declaration style though:

typedef struct T T;
struct T {
   /* stuff here */
};

Reasons why this is used/needed:

(1) structs pointing to each other, like this:

typedef struct A A;
typedef struct B B;

struct A {
   B *b;
};
struct B {
   A *a;
};

(2) keep the struct private, let others pass around pointers
     to the struct in a typesafe way though:

header file:

typedef struct T T;
T* get_foo(args);
int do_something_with_foo(T*);

source file:

struct T {
   /* stuff here */
};

cheers,
   Gerd
Markus Armbruster - Aug. 26, 2009, 3:01 p.m.
Stefan Weil <weil@mail.berlios.de> writes:

> Markus Armbruster schrieb:
>> Stefan Weil <Stefan.Weil@weilnetz.de> writes:
>>
>>> Juan Quintela schrieb:
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>> hw/eepro100.c | 6 +++---
>>>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>>> index 0031d36..09083c2 100644
>>>> --- a/hw/eepro100.c
>>>> +++ b/hw/eepro100.c
>>>> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>>>>
>>>> static void nic_reset(void *opaque)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> logout("%p\n", s);
>>>> static int first;
>>>> if (!first) {
>>>> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState
>>>> *vc, const uint8_t * buf, size_t size
>>>>
>>>> static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> int i;
>>>> int ret;
>>>>
>>>> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void
>>>> *opaque, int version_id)
>>>>
>>>> static void nic_save(QEMUFile * f, void *opaque)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> int i;
>>>>
>>>> if (s->pci_dev)
>>>>
>>> I wrote these type casts, and I think they make sense.
>>> In C++ code, they are even mandatory.
>>
>> Yes, but this isn't C++.
>>
>>> I think the arguments why C++ requires this kind of
>>> type casts apply to C code (like in Qemu) as well.
>>>
>>> If it is possible with no or very litte efforts to write
>>> code which is C and C++ compatible, I prefer to do so.
>>
>> I respectfully disagree. Casts from "void *" to "T *" are pure noise.
>> Getting into the habit of writing noise casts runs the risk of silencing
>> warnings that flag real type errors.
>
> Hello
>
> Do you only disagree with my first sentence or with both sentences?

We can certainly discuss how to write better C, but appealing to the
authority of the C++ rationale is an argument I don't buy.

I prefer not to attempt to write C code that behaves identically when
interpreted as C++ (extern "C" seems safer and easier).

Does this answer your question?

> Currently, I seem to be alone with my opinion (at least in qemu-devel).
> Nevertheless, the designers of C++ thought that casts from void * to
> T * were something very important. I don't know the history of their
> decision. I personally think that deriving a data type T from some
> bytes in memory which can contain anything is an operation which is
> worth being documented by the programmer, and this is exactly what
> the cast does.

I don't see much value in that, sorry.

> My main reason why I try to write portable code is my personal
> experience with code reuse and the future development of compilers.
> It is quite possible that some day C compilers will require
> type casts like C++ compilers do today.

I can't read the ISO C committee's mind, but I believe such an
incompatible change is extremely unlikely, given the pains they've taken
not to break existing C code.

> And even today I want to be able to share C code from QEMU with
> program code written in C++ (which makes a large part of my
> business work).

How about extern "C"?

[...]
Anthony Liguori - Aug. 26, 2009, 3:19 p.m.
Markus Armbruster wrote:
> Stefan Weil <weil@mail.berlios.de> writes:
>   
> We can certainly discuss how to write better C, but appealing to the
> authority of the C++ rationale is an argument I don't buy.
>   
C++ compatibility is not a goal of QEMU AFAIK.

We make extensive use of the fact that you can use struct Foo or Foo as 
a declaration via the TAILQ_ENTRY macro.  With your proposal, that macro 
would no longer work.

Regards,

Anthony Liguori
Måns Rullgård - Aug. 26, 2009, 3:20 p.m.
Stefan Weil <weil@mail.berlios.de> writes:

> Markus Armbruster schrieb:
>> Stefan Weil <Stefan.Weil@weilnetz.de> writes:
>>
>>> Juan Quintela schrieb:
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>> hw/eepro100.c | 6 +++---
>>>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>>> index 0031d36..09083c2 100644
>>>> --- a/hw/eepro100.c
>>>> +++ b/hw/eepro100.c
>>>> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>>>>
>>>> static void nic_reset(void *opaque)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> logout("%p\n", s);
>>>> static int first;
>>>> if (!first) {
>>>> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState
>>>> *vc, const uint8_t * buf, size_t size
>>>>
>>>> static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> int i;
>>>> int ret;
>>>>
>>>> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void
>>>> *opaque, int version_id)
>>>>
>>>> static void nic_save(QEMUFile * f, void *opaque)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> int i;
>>>>
>>>> if (s->pci_dev)
>>>>
>>> I wrote these type casts, and I think they make sense.
>>> In C++ code, they are even mandatory.
>>
>> Yes, but this isn't C++.
>>
>>> I think the arguments why C++ requires this kind of
>>> type casts apply to C code (like in Qemu) as well.
>>>
>>> If it is possible with no or very litte efforts to write
>>> code which is C and C++ compatible, I prefer to do so.
>>
>> I respectfully disagree. Casts from "void *" to "T *" are pure noise.
>> Getting into the habit of writing noise casts runs the risk of silencing
>> warnings that flag real type errors.
>
> Hello
>
> Do you only disagree with my first sentence or with both sentences?
>
> Currently, I seem to be alone with my opinion (at least in qemu-devel).
> Nevertheless, the designers of C++ thought that casts from void * to
> T * were something very important. I don't know the history of their
> decision.

I don't know the history of C++ either.  What I do know is that the
void* type was added to the C language precisely to *avoid* such
casts.  The original K&R C used char* as return type for malloc() etc,
and this of course required a cast.

> I personally think that deriving a data type T from some bytes in
> memory which can contain anything is an operation which is worth
> being documented by the programmer, and this is exactly what the
> cast does.

The declaration and assignment already make that perfectly clear.  The
cast is at best noise, and often hides a real error.

> My main reason why I try to write portable code is my personal
> experience with code reuse and the future development of compilers.
> It is quite possible that some day C compilers will require
> type casts like C++ compilers do today.

Any syntax or semantics can potentially change in the future.  If *I*
were in charge, I'd make frivolous casts an *error*.

> And even today I want to be able to share C code from QEMU with
> program code written in C++ (which makes a large part of my
> business work).

That's your problem.  Please don't bring it here.

> Let me give one more C/C++ example. Today, many data structures
> are declared like this: typedef struct T { ... } T;
> There is nothing wrong with it, but it can be improved in
> several ways:
>
> * The declaration does not work with C++ (yes, I know that many
>   programmers are not interested in C++ compatibility for QEMU).

Nor does it work in Fortran or COBOL.

> * The declaration allows variable declarations using struct T var;
>   or just T var; (which is the QEMU style). I think a declaration
>   which does not enforce the correct style is less good.
>
> * The declaration contains noise (bad argument, but many people
>   like speaking of noise) :-)
>
> Why don't we declare structures like this: typedef struct { ... } T;?
> I suggest this to be the new coding style for structure declarations
> because it is shorter, C++ compatible and unambiguous.

In my personal projects, I never use typedefs for structs.  Typing
those 5 characters isn't much of a burden, and it makes it
unambiguously clear that something is a struct and not something
else.  It even works in C++, should you care.

C is not C++, even though much of the syntax is confusingly similar.
Trying to shoehorn the same coding style onto both is a mistake, and
does more harm than good whichever of the languages you are writing
in.
Avi Kivity - Aug. 26, 2009, 3:54 p.m.
On 08/26/2009 04:52 PM, Stefan Weil wrote:
>>> I wrote these type casts, and I think they make sense.
>>> In C++ code, they are even mandatory.
>>>        
>> Yes, but this isn't C++.
>>
>>      
>>> I think the arguments why C++ requires this kind of
>>> type casts apply to C code (like in Qemu) as well.
>>>
>>> If it is possible with no or very litte efforts to write
>>> code which is C and C++ compatible, I prefer to do so.
>>>        
>> I respectfully disagree. Casts from "void *" to "T *" are pure noise.
>> Getting into the habit of writing noise casts runs the risk of silencing
>> warnings that flag real type errors.
>>      
> Hello
>
> Do you only disagree with my first sentence or with both sentences?
>
> Currently, I seem to be alone with my opinion (at least in qemu-devel).
>    

You are not alone.

> Nevertheless, the designers of C++ thought that casts from void * to
> T * were something very important. I don't know the history of their
> decision. I personally think that deriving a data type T from some
> bytes in memory which can contain anything is an operation which is
> worth being documented by the programmer, and this is exactly what
> the cast does.
>    

Yes.

> Let me give one more C/C++ example. Today, many data structures
> are declared like this: typedef struct T { ... } T;
> There is nothing wrong with it, but it can be improved in
> several ways:
>
> * The declaration does not work with C++ (yes, I know that many
>    programmers are not interested in C++ compatibility for QEMU).
>    

It does work in C++.

> * The declaration allows variable declarations using struct T var;
>    or just T var; (which is the QEMU style). I think a declaration
>    which does not enforce the correct style is less good.
>    

It's often needed, for example to have a struct T * in T.
Reimar Döffinger - Aug. 26, 2009, 3:58 p.m.
On Wed, Aug 26, 2009 at 04:20:14PM +0100, Måns Rullgård wrote:
> Stefan Weil <weil@mail.berlios.de> writes:
> > I personally think that deriving a data type T from some bytes in
> > memory which can contain anything is an operation which is worth
> > being documented by the programmer, and this is exactly what the
> > cast does.
> 
> The declaration and assignment already make that perfectly clear.  The
> cast is at best noise, and often hides a real error.

There are additional points, having all those casts makes people used to
them and generally makes it much harder to spot those that are just
wrong (not that C++ does not have this issue, since basically all
conversions/uses of void* are wrong, with C they are unavoidable).
Having casts for malloc results also makes it needlessly hard to change
the type. Use
var = malloc(count * sizeof(*var))
and you only need to change the declaration, add a cast and you also
have to change all places where such allocations are done.
Avi Kivity - Aug. 26, 2009, 4:08 p.m.
On 08/26/2009 06:58 PM, Reimar Döffinger wrote:
>
> There are additional points, having all those casts makes people used to
> them and generally makes it much harder to spot those that are just
> wrong (not that C++ does not have this issue, since basically all
> conversions/uses of void* are wrong, with C they are unavoidable).
> Having casts for malloc results also makes it needlessly hard to change
> the type. Use
> var = malloc(count * sizeof(*var))
> and you only need to change the declaration, add a cast and you also
> have to change all places where such allocations are done.
>    

Or better, NEW() and NEW_ARRAY().
Jamie Lokier - Aug. 26, 2009, 5:04 p.m.
Gerd Hoffmann wrote:
>   Hi,
> 
> >Why don't we declare structures like this: typedef struct { ... } T;?
> >I suggest this to be the new coding style for structure declarations
> >because it is shorter, C++ compatible and unambiguous.
> 
> There are quite a few cases where this will simply not work.  They 
> usually use a slightly different declaration style though:
...
> (1) structs pointing to each other, like this:
> 
> typedef struct A A;
> typedef struct B B;

You can use "typedef struct _A A" to be C++ compatible, but it fails
to be shorter so I wouldn't recommend it ;-)

-- Jamie
Jamie Lokier - Aug. 26, 2009, 5:36 p.m.
Avi Kivity wrote:
> On 08/26/2009 04:52 PM, Stefan Weil wrote:
> >Nevertheless, the designers of C++ thought that casts from void * to
> >T * were something very important. I don't know the history of their
> >decision. I personally think that deriving a data type T from some
> >bytes in memory which can contain anything is an operation which is
> >worth being documented by the programmer, and this is exactly what
> >the cast does.
> 
> Yes.

Not really.  Adding the cast can hide bugs.

If you omit the cast, a C compiler will at least warn, and maybe
error, if the original pointer is not "void *", or is "const void *"
and you are removing the constness.

With the cast, the C compiler will silently let you compile buggy code.

In C++, results vary.  You are supposed to use a base class pointer,
and if you need a cast, you are supposed to use "static_cast<>"
anyway.

In C++, it's an error without the cast, but it can also be an error
with the cast:

    g++ -Wall test.cc -c -Werror=old-style-cast
    test.cc: In function ‘S* foo(void*)’:
    test.cc:5: error: use of old-style cast

    struct S *foo(void *ptr)
    {
        return (struct S *)ptr;
    }

Of course there are dirty tricks to let you omit the "void *" pointer
altogether.  Let's not go there :-)

-- Jamie
malc - Aug. 26, 2009, 6:37 p.m.
On Wed, 26 Aug 2009, Jamie Lokier wrote:

> Gerd Hoffmann wrote:
> >   Hi,
> > 
> > >Why don't we declare structures like this: typedef struct { ... } T;?
> > >I suggest this to be the new coding style for structure declarations
> > >because it is shorter, C++ compatible and unambiguous.
> > 
> > There are quite a few cases where this will simply not work.  They 
> > usually use a slightly different declaration style though:
> ..
> > (1) structs pointing to each other, like this:
> > 
> > typedef struct A A;
> > typedef struct B B;
> 
> You can use "typedef struct _A A" to be C++ compatible, but it fails
> to be shorter so I wouldn't recommend it ;-)

This is neither C nor C++ compatible, in fact it breaks both.
Jamie Lokier - Aug. 26, 2009, 7:03 p.m.
malc wrote:
> > > (1) structs pointing to each other, like this:
> > > 
> > > typedef struct A A;
> > > typedef struct B B;
> > 
> > You can use "typedef struct _A A" to be C++ compatible, but it fails
> > to be shorter so I wouldn't recommend it ;-)
> 
> This is neither C nor C++ compatible, in fact it breaks both.

You'll have to explain that statement.

It compiles fine for me in C and C++:

   typedef struct _A A;
   typedef struct _B B;

   struct _A { struct _B *b; };
   struct _B { struct _A *a; };

   A *foo(B *arg);

Using this works equally well:

   struct _A { B *b; };
   struct _B { A *a; };

-- Jamie
malc - Aug. 26, 2009, 7:26 p.m.
On Wed, 26 Aug 2009, Jamie Lokier wrote:

> malc wrote:
> > > > (1) structs pointing to each other, like this:
> > > > 
> > > > typedef struct A A;
> > > > typedef struct B B;
> > > 
> > > You can use "typedef struct _A A" to be C++ compatible, but it fails
> > > to be shorter so I wouldn't recommend it ;-)
> > 
> > This is neither C nor C++ compatible, in fact it breaks both.
> 
> You'll have to explain that statement.

ISO/IEC 9899:1999 7.1.3#1
and 17.4.3.1.2 of n2315 (Draft of c++0x)

mandate that those names are reserved.

[..snip..]
Jamie Lokier - Aug. 26, 2009, 9 p.m.
malc wrote:
> On Wed, 26 Aug 2009, Jamie Lokier wrote:
> > malc wrote:
> > > > > (1) structs pointing to each other, like this:
> > > > > 
> > > > > typedef struct A A;
> > > > > typedef struct B B;
> > > > 
> > > > You can use "typedef struct _A A" to be C++ compatible, but it fails
> > > > to be shorter so I wouldn't recommend it ;-)
> > > 
> > > This is neither C nor C++ compatible, in fact it breaks both.
> > 
> > You'll have to explain that statement.
> 
> ISO/IEC 9899:1999 7.1.3#1
> and 17.4.3.1.2 of n2315 (Draft of c++0x)
> 
> mandate that those names are reserved.

The _A is just an example; just substitute an identifier you're happy
with.  Use lower case, or put the underscore at the end, or something else.

It's actively unhelpful to say "that is not C/C++ compatible" without
explaining that it's for an tangential reason which is easily avoided.

-- Jamie
Stuart Brady - Sept. 3, 2009, 12:05 p.m.
On Wed, Aug 26, 2009 at 07:08:55PM +0300, Avi Kivity wrote:
> Or better, NEW() and NEW_ARRAY().

ISTR this being discussed before, but there was some disagreement
regarding whether it is preferable to have:

   QEMU_NEW(ptr);

or:

   ptr = QEMU_NEW(type);

If the type is incorrect, the latter form would still at least yield a
warning (and now therefore a build failure).  It seems slightly more 
readable to me, so that's the form that I would have preferred...

Is there any reason that this wouldn't be accepted, or should I start
submitting patches?

Cheers,

Patch

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 0031d36..09083c2 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -507,7 +507,7 @@  static void nic_selective_reset(EEPRO100State * s)

 static void nic_reset(void *opaque)
 {
-    EEPRO100State *s = (EEPRO100State *) opaque;
+    EEPRO100State *s = opaque;
     logout("%p\n", s);
     static int first;
     if (!first) {
@@ -1544,7 +1544,7 @@  static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size

 static int nic_load(QEMUFile * f, void *opaque, int version_id)
 {
-    EEPRO100State *s = (EEPRO100State *) opaque;
+    EEPRO100State *s = opaque;
     int i;
     int ret;

@@ -1634,7 +1634,7 @@  static int nic_load(QEMUFile * f, void *opaque, int version_id)

 static void nic_save(QEMUFile * f, void *opaque)
 {
-    EEPRO100State *s = (EEPRO100State *) opaque;
+    EEPRO100State *s = opaque;
     int i;

     if (s->pci_dev)