Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Kent Harris]

Stefan,

I'm with you 100 percent.  I already complained to this group a couple  
of months ago about the void* casting issue (and also using "private"  
as a structure element name as that barfs C++ as well -- there are  
others.)

For my situation, these problems within C-language source files are  
not an issue but when they occur inside a QEMU header file that I must  
include from my C++ device models, my compilations fail.   
Consequently, I have a whole series of patch files to the QEMU source  
that I have to apply to correct these (trivial) coding errors.  Each  
time a new QEMU release comes out, I have to recreate the patch  
files.  Frustrating to say the least.

An old platitude says it best;  Slovenly writing indulges the writer  
while clear writing indulges the reader.

Regards,

Kent

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Måns Rullgård]

Kent Harris <ksh@vine.com> writes:

> Stefan,
>
> I'm with you 100 percent.  I already complained to this group a couple
> of months ago about the void* casting issue (and also using "private"
> as a structure element name as that barfs C++ as well -- there are
> others.)
>
> For my situation, these problems within C-language source files are
> not an issue but when they occur inside a QEMU header file that I must
> include from my C++ device models, my compilations fail.
> Consequently, I have a whole series of patch files to the QEMU source
> that I have to apply to correct these (trivial) coding errors.  Each
> time a new QEMU release comes out, I have to recreate the patch
> files.  Frustrating to say the least.

#define private cxx_private
#include <qemu.h>
#undef  private

Job done.

-- 
Måns Rullgård
mans@mansr.com

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Måns Rullgård]

Kent Harris <ksh@vine.com> writes:

> Stefan,
>
> I'm with you 100 percent.  I already complained to this group a couple
> of months ago about the void* casting issue (and also using "private"
> as a structure element name as that barfs C++ as well -- there are
> others.)
>
> For my situation, these problems within C-language source files are
> not an issue but when they occur inside a QEMU header file that I must
> include from my C++ device models, my compilations fail.
> Consequently, I have a whole series of patch files to the QEMU source
> that I have to apply to correct these (trivial) coding errors.  Each
> time a new QEMU release comes out, I have to recreate the patch
> files.  Frustrating to say the least.

I have a great idea.  For maximum portability, lets all start writing
code like this: http://ideology.com.au/polyglot/polyglot.txt That way,
it will work in almost any language, and everybody will be happy.

-- 
Måns Rullgård
mans@mansr.com

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[malc]

On Thu, 27 Aug 2009, M?ns Rullg?rd wrote:

> Kent Harris <ksh@vine.com> writes:
> 
> > Stefan,
> >
> > I'm with you 100 percent.  I already complained to this group a couple
> > of months ago about the void* casting issue (and also using "private"
> > as a structure element name as that barfs C++ as well -- there are
> > others.)
> >
> > For my situation, these problems within C-language source files are
> > not an issue but when they occur inside a QEMU header file that I must
> > include from my C++ device models, my compilations fail.
> > Consequently, I have a whole series of patch files to the QEMU source
> > that I have to apply to correct these (trivial) coding errors.  Each
> > time a new QEMU release comes out, I have to recreate the patch
> > files.  Frustrating to say the least.
> 
> I have a great idea.  For maximum portability, lets all start writing
> code like this: http://ideology.com.au/polyglot/polyglot.txt That way,
> it will work in almost any language, and everybody will be happy.

Reminds me of http://fly.srk.fer.hr/ioccc/1986/applin.c

-- 
mailto:av1474@comtv.ru

[Qemu-devel] [PATCH 00/22] Indirection Cleanup

[Juan Quintela]

This patch series clean up "half" converted qemu drivers that had changed from:

struct FOOState

to

typedef PCIFOOState {
    PCIDevice dev;
    FOOState foo;
} PCIFOOState;

It just moves PCIDevice to be the 1st field of FOOState.

Once there, other cleanups were done:
a - pci_dev pointer from FOOState to PCIFOOState is removed, jsut use s->dev
    The field is leave only in the drivers that also emulate isa.
b- Once there, transformo
   PCIFOOState *s = (PCIFOOState *)pci_dev
   to
   PCIFOOState *s = DO_UPCAST(PCIFOOState, dev, pci_dev)
   where pci_dev is a PCI_DEVICE.
c- again, once there, remove all the casts from void * (they are not
   needed since '89)
   PCIFOOState *s = (PCIFOOState *)opaque;
   to
   PCIFOOState *s = opaque;
d- Start of vga.c cleanup.  It is not trivial, as just now
   VGAState == VGACommonState, functions need to be changed to use the right value.

ToDo:
- pcnet:  It needs a different approach, because it can be both a PCIDevice
  or a SysBus device.
- vga: It have to separate the common part for the not common part, problems now
  is that VGAState is used for both the common state and the standard vga.
  To make things more interesting, different bits are "inherited" from vga
  in different devices:
  - cirrus_vga
  - vmware_vga
  - blizzard (vga that is not pci)
  - vga common state has a pci_dev pointer, that is only needed by std vga,
    as cirrus_vga stores it (with this patches) otherplace, blizzard is not pci
    .... you get the idea
- I guess some other driver is missing, but my fast grep didn't found it.

Later, Juan.


Juan Quintela (22):
  eepro100: convert casts to DO_UPCAST()
  eepro100: cast a void * makes no sense
  eepro100: Remove unused indirection of PCIDevice
  es1370: Remove unused indirection of PCIES1370State and ES1370State
  ne2000: remove casts from void *
  ne2000: pci_dev has this very value with the right type
  ne2000: Remove unneeded double indirection of PCINE2000State
  ne2000: change pci_dev to is_pci
  pci: remove casts from void *
  rtl8139: Remove unneeded double indirection of PCIRTL8139State
  rtl8139: remove pointless cast from void *
  lsi53c895a: remove pointless cast from void *
  lsi53c895a: use DO_UPCAST to cast from PCIDevice
  lsi53c895a: rename PCIDevice field from pci_dev to dev (consistence)
  lsi53c895a: LSIState is a PCIDevice is a DeviceHost
  usb-ohci: Remove unneeded double indirection of OHCIPCIState
  cirrus_vga: Remove unneeded double indirection of PCICirrusVGAState
  cirrus_vga: remove pointless cast from void *
  cirrus_vga: change use of pci_dev for is_pci
  Introduce vga_common_reset() to be able to typcheck vga_reset()
  vga: Rename vga_state -> vga
  Everything outside of vga.c should use VGACommonState

 hw/cirrus_vga.c |   59 ++++++++++++++++++++++---------------------------
 hw/eepro100.c   |   65 ++++++++++++++++++++++--------------------------------
 hw/es1370.c     |   31 ++++++++-----------------
 hw/lsi53c895a.c |   65 ++++++++++++++++++++++++++++---------------------------
 hw/ne2000.c     |   41 ++++++++++++++--------------------
 hw/pci.c        |    8 +++---
 hw/rtl8139.c    |   42 ++++++++++++----------------------
 hw/usb-ohci.c   |   34 +++++++++++-----------------
 hw/vga.c        |   26 +++++++++++++--------
 hw/vga_int.h    |   12 ++++------
 hw/vmware_vga.c |    4 +-
 11 files changed, 170 insertions(+), 217 deletions(-)

[Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense

[Juan Quintela]

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)
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense

[Stefan Weil]

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

Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense

[Markus Armbruster]

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.

[Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Stefan Weil]

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

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Måns Rullgård]

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.

-- 
Måns Rullgård
mans@mansr.com

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Reimar Döffinger]

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.

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Avi Kivity]

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().

-- 
error compiling committee.c: too many arguments to function

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Stuart Brady]

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,
-- 
Stuart Brady