qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 01/14] Add new data type for fprintf like functi


From: Stefan Weil
Subject: Re: [Qemu-devel] [PATCH 01/14] Add new data type for fprintf like function pointers
Date: Thu, 01 Jul 2010 11:08:49 +0200
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.10) Gecko/20100620 Iceowl/1.0b1 Icedove/3.0.5

Am 09.04.2010 13:20, schrieb Stefan Weil:
Aurelien Jarno schrieb:
On Mon, Mar 29, 2010 at 09:16:52PM +0200, Stefan Weil wrote:
The compiler should check the arguments for these functions.

gcc can do this, but only if the function pointer's prototype
includes the __attribute__ flag.

As the necessary declaration is a bit lengthy, we use a new
data type 'fprintf_function'.

It is not easy to find a single header file which is included
everywhere, so fprint_function had to be declared in several
header files.

I don't really think it is a good idea to duplicate that. It will only
causes problem in the future. Are you sure there is no header for that?
Worst case scenario it's probably better to create a new header.

I had no better idea. As long as the duplicate declarations
always observe the same pattern, they should not really cause
problems. Anybody who knows this pattern (which is also quite
common in system include files) will know that there are duplicates.

I did not want to create a new header because it is really a worst
case scenario with several disadvantages.

In the meantime I noticed that dis-asm.h also uses fprintf like
function pointers, so there is one more header which needs
the same declaration.

Maybe the best solution would be using qemu-common.h in
cpu-exec.c, *-dis.c, */translate.c, and more files.
That would involve a lot of modifications, for example
removing code which re-implements parts of stdio.h in
dyngen-exec.h. Some restrictions why qemu-common.h was
not used might be no longer valid (I think they came
from pre-tcg times). Nevertheless, cris-dis.c even says
that it cannot include qemu-common.h (without giving a
reason). Reordering include statements or adding new
includes can have unwanted side effects which are
difficult to detect.

So this last solution needs a lot of discussion and time.
That's the reason why I did not choose it. Maybe I was wrong
and more developers want to clean up includes, so it can be done.

More files use qemu-common.h now, so it seems possible to declare
the new data type only once (in qemu-common.h).

There are undetected format errors in current code. Without
argument checking by the compiler, even new format errors
are introduced from time to time. Therefore the motivation
for these patches is still given.

Before I send updated patches, I'd like to ask a simple question:

There is already a data type named 'fprintf_type' (without __attribute__
flag) which is only used in *-dis.c. So which name is preferred
for the new data type with __attribute__ flag?

* fprintf_type (already used in *-dis.c)?
* fprintf_function (which I used in my patches)
* any other name (coding rules?)





reply via email to

[Prev in Thread] Current Thread [Next in Thread]