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

[Aurelien Jarno <aurelien@aurel32.net>]

On Thu, Jul 01, 2010 at 11:08:49AM +0200, Stefan Weil wrote:
> 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)?

It is actually fprintf_ftype.

> * fprintf_function (which I used in my patches)
> * any other name (coding rules?)

I am personally fine with fprintf_function.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net