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.

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

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

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

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

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.

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.

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.

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

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

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

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.

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,