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

[Stefan Weil <weil@mail.berlios.de>]

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