[Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

Hi,

I made a series of patches that add -Wstrict-prototypes to the CFLAGS
and then -Wmissing-prototypes, both of which are enabled by Xen. I
also fixed most warnings generated -Wstrict-prototypes and some of
them for the -Wmissing-prototypes case.

Compiling with -Wstrict-prototypes produces only one extra warning. I
think this flag should be enabled.

But dyngen targets spew a lot of noise with -Wmissing-prototypes so
I'm not proposing to add that (or maybe that should be coupled with
CONFIG_DYNGEN_OP). This also meant that I did not care enough to fix
all warnings. The current warning fix should be safe to apply anyway.

Comments?

[-- Attachment #2: wstrict_prototypes.diff --]
[-- Type: plain/text, Size: 1007 bytes --]

[-- Attachment #3: fix_wstrict_prototypes_warnings.diff --]
[-- Type: plain/text, Size: 2876 bytes --]

[-- Attachment #4: wmissing_prototypes.diff --]
[-- Type: plain/text, Size: 1089 bytes --]

[-- Attachment #5: fix_wmissing_prototypes_warnings.diff --]
[-- Type: plain/text, Size: 15100 bytes --]

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Anthony Liguori]

Blue Swirl wrote:
> Hi,
>
> I made a series of patches that add -Wstrict-prototypes to the CFLAGS
> and then -Wmissing-prototypes, both of which are enabled by Xen. I
> also fixed most warnings generated -Wstrict-prototypes and some of
> them for the -Wmissing-prototypes case.
>
> Compiling with -Wstrict-prototypes produces only one extra warning. I
> think this flag should be enabled.
>   

As long as the plan is to fix all of those warnings, I think it's a good 
idea.

Regards,

Anthony Liguori

> But dyngen targets spew a lot of noise with -Wmissing-prototypes so
> I'm not proposing to add that (or maybe that should be coupled with
> CONFIG_DYNGEN_OP). This also meant that I did not care enough to fix
> all warnings. The current warning fix should be safe to apply anyway.
>
> Comments?
>   

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Blue Swirl]

On 8/9/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Blue Swirl wrote:
>
> > Hi,
> >
> > I made a series of patches that add -Wstrict-prototypes to the CFLAGS
> > and then -Wmissing-prototypes, both of which are enabled by Xen. I
> > also fixed most warnings generated -Wstrict-prototypes and some of
> > them for the -Wmissing-prototypes case.
> >
> > Compiling with -Wstrict-prototypes produces only one extra warning. I
> > think this flag should be enabled.
> >
> >
>
>  As long as the plan is to fix all of those warnings, I think it's a good
> idea.

The extra unfixed warning comes from monitor.c:
typedef struct term_cmd_t {
    const char *name;
    const char *args_type;
    void (*handler)();
    const char *params;
    const char *help;
} term_cmd_t;

The warning is generated because the definition of "handler" should
also describe the parameters and not use the old () style. But in this
case, they can vary:
static void do_help(const char *name)
static void do_quit(void)
static void do_eject(int force, const char *filename)
static void do_change(const char *device, const char *target, const char *fmt)
static void do_screen_dump(const char *filename)
static void do_memory_dump(int count, int format, int size,
                           uint32_t addrh, uint32_t addrl)
static void do_print(int count, int format, int size, unsigned int
valh, unsigned int vall)
static void do_ioport_read(int count, int format, int size, int addr,
int has_index, int index)
etc.

I don't have a good plan how to fix this, proposals are welcome.
Changing all handlers to use va_args to just silence a gcc warning
sounds like overkill.

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Gerd Hoffmann]

Blue Swirl wrote:
> On 8/9/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
>>  As long as the plan is to fix all of those warnings, I think it's a good
>> idea.
> 
> The extra unfixed warning comes from monitor.c:
> typedef struct term_cmd_t {
>     const char *name;
>     const char *args_type;
>     void (*handler)();
>     const char *params;
>     const char *help;
> } term_cmd_t;
> 
> The warning is generated because the definition of "handler" should
> also describe the parameters and not use the old () style. But in this
> case, they can vary:
> static void do_help(const char *name)
> static void do_quit(void)
[ ... ]

> I don't have a good plan how to fix this, proposals are welcome.
> Changing all handlers to use va_args to just silence a gcc warning
> sounds like overkill.

Using a union maybe?

typedef struct term_cmd_t {
      [ ... ]
      union {
          void (*help)(const char name);
          void (*quit)(void);
          [ ... ]
     } handlers;
     [ ... ]
};

cheers,
  Gerd


-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Samuel Thibault]

Gerd Hoffmann, le Mon 11 Aug 2008 09:52:03 +0200, a écrit :
> > The warning is generated because the definition of "handler" should
> > also describe the parameters and not use the old () style. But in this
> > case, they can vary:
> > static void do_help(const char *name)
> > static void do_quit(void)
> [ ... ]
> 
> > I don't have a good plan how to fix this, proposals are welcome.
> > Changing all handlers to use va_args to just silence a gcc warning
> > sounds like overkill.
> 
> Using a union maybe?

That should be the right thing yes.

Samuel

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[M. Warner Losh]

In message: <489FEFA3.3040105@redhat.com>
            Gerd Hoffmann <kraxel@redhat.com> writes:
: Blue Swirl wrote:
: > On 8/9/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
: >>  As long as the plan is to fix all of those warnings, I think it's a good
: >> idea.
: > 
: > The extra unfixed warning comes from monitor.c:
: > typedef struct term_cmd_t {
: >     const char *name;
: >     const char *args_type;
: >     void (*handler)();
: >     const char *params;
: >     const char *help;
: > } term_cmd_t;
: > 
: > The warning is generated because the definition of "handler" should
: > also describe the parameters and not use the old () style. But in this
: > case, they can vary:
: > static void do_help(const char *name)
: > static void do_quit(void)
: [ ... ]
: 
: > I don't have a good plan how to fix this, proposals are welcome.
: > Changing all handlers to use va_args to just silence a gcc warning
: > sounds like overkill.
: 
: Using a union maybe?
: 
: typedef struct term_cmd_t {
:       [ ... ]
:       union {
:           void (*help)(const char name);
:           void (*quit)(void);
:           [ ... ]
:      } handlers;
:      [ ... ]
: };

Or just have all the commands use the same arguments?  That's usually
how this is done.  (*handler) should take a void *, which the callee
can do whatever it wants with...

Warner

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[M. Warner Losh]

In message: <20080811095457.GA4499@implementation.uk.xensource.com>
            Samuel Thibault <samuel.thibault@ens-lyon.org> writes:
: Gerd Hoffmann, le Mon 11 Aug 2008 09:52:03 +0200, a écrit :
: > > The warning is generated because the definition of "handler" should
: > > also describe the parameters and not use the old () style. But in this
: > > case, they can vary:
: > > static void do_help(const char *name)
: > > static void do_quit(void)
: > [ ... ]
: > 
: > > I don't have a good plan how to fix this, proposals are welcome.
: > > Changing all handlers to use va_args to just silence a gcc warning
: > > sounds like overkill.
: > 
: > Using a union maybe?
: 
: That should be the right thing yes.

Except it becomes unmanageable over time...  It also doesn't enhance
type safety...

Warner

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Anthony Liguori]

Blue Swirl wrote:
> On 8/9/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
>   
>> Blue Swirl wrote:
>>
>>     
>>> Hi,
>>>
>>> I made a series of patches that add -Wstrict-prototypes to the CFLAGS
>>> and then -Wmissing-prototypes, both of which are enabled by Xen. I
>>> also fixed most warnings generated -Wstrict-prototypes and some of
>>> them for the -Wmissing-prototypes case.
>>>
>>> Compiling with -Wstrict-prototypes produces only one extra warning. I
>>> think this flag should be enabled.
>>>
>>>
>>>       
>>  As long as the plan is to fix all of those warnings, I think it's a good
>> idea.
>>     
>
> The extra unfixed warning comes from monitor.c:
> typedef struct term_cmd_t {
>     const char *name;
>     const char *args_type;
>     void (*handler)();
>     const char *params;
>     const char *help;
> } term_cmd_t;
>
> The warning is generated because the definition of "handler" should
> also describe the parameters and not use the old () style. But in this
> case, they can vary:
>   

You could just switch void (*handler)() to void *handler.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Avi Kivity]

Anthony Liguori wrote:
> Blue Swirl wrote:
>> On 8/9/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
>>  
>>> Blue Swirl wrote:
>>>
>>>    
>>>> Hi,
>>>>
>>>> I made a series of patches that add -Wstrict-prototypes to the CFLAGS
>>>> and then -Wmissing-prototypes, both of which are enabled by Xen. I
>>>> also fixed most warnings generated -Wstrict-prototypes and some of
>>>> them for the -Wmissing-prototypes case.
>>>>
>>>> Compiling with -Wstrict-prototypes produces only one extra warning. I
>>>> think this flag should be enabled.
>>>>
>>>>
>>>>       
>>>  As long as the plan is to fix all of those warnings, I think it's a 
>>> good
>>> idea.
>>>     
>>
>> The extra unfixed warning comes from monitor.c:
>> typedef struct term_cmd_t {
>>     const char *name;
>>     const char *args_type;
>>     void (*handler)();
>>     const char *params;
>>     const char *help;
>> } term_cmd_t;
>>
>> The warning is generated because the definition of "handler" should
>> also describe the parameters and not use the old () style. But in this
>> case, they can vary:
>>   
>
> You could just switch void (*handler)() to void *handler. 

Function pointers might be wider than data pointers.

Looking at the usage, though, it looks like the best thing is to have a

  void (*handler)(const char **args)

So we pass the array of arguments (could be zero length) to the handler.

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

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Paul Brook]

> > You could just switch void (*handler)() to void *handler.
>
> Function pointers might be wider than data pointers.

I'm pretty sure this isn't true on any current system. On some targets you end 
up using a function descriptor, but it should still work.

> void (*handler)(const char **args)

This seems like a reasonable idea. None of this code is performance critical.

Paul

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Laurent Vivier]

Le lundi 11 août 2008 à 17:48 +0300, Avi Kivity a écrit :
> Anthony Liguori wrote:
> > Blue Swirl wrote:
> >> On 8/9/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
> >>  
> >>> Blue Swirl wrote:
> >>>
> >>>    
> >>>> Hi,
> >>>>
> >>>> I made a series of patches that add -Wstrict-prototypes to the CFLAGS
> >>>> and then -Wmissing-prototypes, both of which are enabled by Xen. I
> >>>> also fixed most warnings generated -Wstrict-prototypes and some of
> >>>> them for the -Wmissing-prototypes case.
> >>>>
> >>>> Compiling with -Wstrict-prototypes produces only one extra warning. I
> >>>> think this flag should be enabled.
> >>>>
> >>>>
> >>>>       
> >>>  As long as the plan is to fix all of those warnings, I think it's a 
> >>> good
> >>> idea.
> >>>     
> >>
> >> The extra unfixed warning comes from monitor.c:
> >> typedef struct term_cmd_t {
> >>     const char *name;
> >>     const char *args_type;
> >>     void (*handler)();
> >>     const char *params;
> >>     const char *help;
> >> } term_cmd_t;
> >>
> >> The warning is generated because the definition of "handler" should
> >> also describe the parameters and not use the old () style. But in this
> >> case, they can vary:
> >>   
> >
> > You could just switch void (*handler)() to void *handler. 
> 
> Function pointers might be wider than data pointers.
> 
> Looking at the usage, though, it looks like the best thing is to have a
> 
>   void (*handler)(const char **args)
> 
> So we pass the array of arguments (could be zero length) to the handler.

I agree with that.

Each command can be seen like a shell command and "handler" is like the
"int main(int argc, char **argv)".

Perhaps we can have an "int (*handler)(int argc, char **argv)" ?

Regards,
Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Blue Swirl wrote:
>>> On 8/9/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
>>>  
>>>> Blue Swirl wrote:
>>>>
>>>>   
>>>>> Hi,
>>>>>
>>>>> I made a series of patches that add -Wstrict-prototypes to the CFLAGS
>>>>> and then -Wmissing-prototypes, both of which are enabled by Xen. I
>>>>> also fixed most warnings generated -Wstrict-prototypes and some of
>>>>> them for the -Wmissing-prototypes case.
>>>>>
>>>>> Compiling with -Wstrict-prototypes produces only one extra warning. I
>>>>> think this flag should be enabled.
>>>>>
>>>>>
>>>>>       
>>>>  As long as the plan is to fix all of those warnings, I think it's 
>>>> a good
>>>> idea.
>>>>     
>>>
>>> The extra unfixed warning comes from monitor.c:
>>> typedef struct term_cmd_t {
>>>     const char *name;
>>>     const char *args_type;
>>>     void (*handler)();
>>>     const char *params;
>>>     const char *help;
>>> } term_cmd_t;
>>>
>>> The warning is generated because the definition of "handler" should
>>> also describe the parameters and not use the old () style. But in this
>>> case, they can vary:
>>>   
>>
>> You could just switch void (*handler)() to void *handler. 
>
> Function pointers might be wider than data pointers.

On what platforms?  void (*)() was deprecated as a generic function 
pointer.  IIUC, there is no replacement and the rationale for that is 
that the vast majority of people can just use 'void *'.  The general 
technique is discouraged though.

> Looking at the usage, though, it looks like the best thing is to have a
>
>  void (*handler)(const char **args)
>
> So we pass the array of arguments (could be zero length) to the handler.

Yeah, it uglifies things quite a bit though.  You lose all of the nice 
parsing and lack of casting.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Avi Kivity]

Anthony Liguori wrote:
>>>
>>> You could just switch void (*handler)() to void *handler. 
>>
>> Function pointers might be wider than data pointers.
>
> On what platforms?  void (*)() was deprecated as a generic function 
> pointer.  IIUC, there is no replacement and the rationale for that is 
> that the vast majority of people can just use 'void *'.  The general 
> technique is discouraged though.
>

Segmented x86.  You have function pointers consisting of a 
segment:offset, and data pointers consisting of an offset into the 
implied data segment.  This was one out of five available data models.

As Paul mentioned, no modern machine does that anymore, but the 
distinction lives on in the standard.

>> Looking at the usage, though, it looks like the best thing is to have a
>>
>>  void (*handler)(const char **args)
>>
>> So we pass the array of arguments (could be zero length) to the handler.
>
> Yeah, it uglifies things quite a bit though.  You lose all of the nice 
> parsing and lack of casting. 

You lose the switch(nb_args) in monitor_handle_command() so it comes out 
even.  You don't need to cast anything -- though you do need to change 
all the callbacks to accept an argument array.

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

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Anthony Liguori]

Avi Kivity wrote:
>
> You lose the switch(nb_args) in monitor_handle_command() so it comes 
> out even.  You don't need to cast anything -- though you do need to 
> change all the callbacks to accept an argument array.

And to either explicitly cast each argument or explicitly parse each 
argument.  It touches quite a bit of places in the code and makes the 
callbacks quite a bit more complicated.

Using a void * is a one line change and works on all modern systems.  
Seems like a winner to me :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Blue Swirl]

On 8/11/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Avi Kivity wrote:
>
> >
> > You lose the switch(nb_args) in monitor_handle_command() so it comes out
> even.  You don't need to cast anything -- though you do need to change all
> the callbacks to accept an argument array.
> >
>
>  And to either explicitly cast each argument or explicitly parse each
> argument.  It touches quite a bit of places in the code and makes the
> callbacks quite a bit more complicated.
>
>  Using a void * is a one line change and works on all modern systems.  Seems
> like a winner to me :-)

I vote for this. The other solutions do not improve type safety
(except for union) and they are more complex.

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Laurent Vivier]

Le 11 août 08 à 20:52, Blue Swirl a écrit :

> On 8/11/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> Avi Kivity wrote:
>>
>>>
>>> You lose the switch(nb_args) in monitor_handle_command() so it  
>>> comes out
>> even.  You don't need to cast anything -- though you do need to  
>> change all
>> the callbacks to accept an argument array.
>>>
>>
>> And to either explicitly cast each argument or explicitly parse each
>> argument.  It touches quite a bit of places in the code and makes the
>> callbacks quite a bit more complicated.
>>
>> Using a void * is a one line change and works on all modern  
>> systems.  Seems
>> like a winner to me :-)
>
> I vote for this. The other solutions do not improve type safety
> (except for union) and they are more complex.

But I think to call the function you have to cast the pointer, this  
doesn't improve type safety too...

Regards,
Laurent
----------------------- Laurent Vivier ----------------------
"The best way to predict the future is to invent it."
- Alan Kay

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Blue Swirl]

On 8/11/08, Laurent Vivier <laurent@lvivier.info> wrote:
>
>  Le 11 août 08 à 20:52, Blue Swirl a écrit :
>
>
>
> > On 8/11/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
> >
> > > Avi Kivity wrote:
> > >
> > >
> > > >
> > > > You lose the switch(nb_args) in monitor_handle_command() so it comes
> out
> > > >
> > > even.  You don't need to cast anything -- though you do need to change
> all
> > > the callbacks to accept an argument array.
> > >
> > > >
> > > >
> > >
> > > And to either explicitly cast each argument or explicitly parse each
> > > argument.  It touches quite a bit of places in the code and makes the
> > > callbacks quite a bit more complicated.
> > >
> > > Using a void * is a one line change and works on all modern systems.
> Seems
> > > like a winner to me :-)
> > >
> >
> > I vote for this. The other solutions do not improve type safety
> > (except for union) and they are more complex.
> >
>
>  But I think to call the function you have to cast the pointer, this doesn't
> improve type safety too...

It doesn't, but it's not too complex considering the original aim of
silencing gcc.

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Anthony Liguori]

Laurent Vivier wrote:
>
>>
>> I vote for this. The other solutions do not improve type safety
>> (except for union) and they are more complex.
>
> But I think to call the function you have to cast the pointer, this 
> doesn't improve type safety too...

Right now, the monitor works by taking a generic function pointer "void 
(*)()" which is treated specially by the C standard (although it's now 
deprecated).  Any function pointer can be cast to a generic function 
pointer.  The dispatch loop basically looks like:

void dispatch(void (*func)(), int n_args, void *args)
{
    switch (n_args) {
    case 1:
        ((void (*)(void *))func)(args[0]); break;
    case 1:
        ((void (*)(void *, void *))func)(args[0]); break;
    ...
}

But this tosses a warning since void (*)() is deprecated.  So, if we use:

void dispatch(void *func, int n_args, void *args)

It'll work just like it did before.

Regards,

Anthony Liguori

>
> Regards,
> Laurent
> ----------------------- Laurent Vivier ----------------------
> "The best way to predict the future is to invent it."
> - Alan Kay
>
>
>
>
>
>
>

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Laurent Vivier]

Le lundi 11 août 2008 à 14:22 -0500, Anthony Liguori a écrit :
> Laurent Vivier wrote:
> >
> >>
> >> I vote for this. The other solutions do not improve type safety
> >> (except for union) and they are more complex.
> >
> > But I think to call the function you have to cast the pointer, this 
> > doesn't improve type safety too...
> 
> Right now, the monitor works by taking a generic function pointer "void 
> (*)()" which is treated specially by the C standard (although it's now 
> deprecated).  Any function pointer can be cast to a generic function 
> pointer.  The dispatch loop basically looks like:
> 
> void dispatch(void (*func)(), int n_args, void *args)
> {
>     switch (n_args) {
>     case 1:
>         ((void (*)(void *))func)(args[0]); break;
>     case 1:
>         ((void (*)(void *, void *))func)(args[0]); break;
>     ...
> }
> 
> But this tosses a warning since void (*)() is deprecated.  So, if we use:
> 
> void dispatch(void *func, int n_args, void *args)
> 
> It'll work just like it did before.
> 

but using "void (*handler)(int argc, char** argv)" avoids the switch:

switch(nb_args) {
    case 0:
        cmd->handler();
        break;
    case 1:
        cmd->handler(args[0]);
        break;
...
}

becomes

cmd->handler(nb_args, args);

Laurent

-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Anthony Liguori]

Laurent Vivier wrote:
> Le lundi 11 août 2008 à 14:22 -0500, Anthony Liguori a écrit :
>   
>
> but using "void (*handler)(int argc, char** argv)" avoids the switch:
>
> switch(nb_args) {
>     case 0:
>         cmd->handler();
>         break;
>     case 1:
>         cmd->handler(args[0]);
>         break;
> ...
> }
>
> becomes
>
> cmd->handler(nb_args, args);
>   

And then every monitor command changes from:

void do_eject(int force, char *device)
{
     ...
}

to:

void do_eject(int argc, char **argv)
{
   char *device;
   int force = 0;

   if (argc == 2) {
     if (strcmp(argv[0], "-f") == 0) {
         force = 1;
         device = argv[1];
     } else {
         term_printf("bad option %s\n", argv[0]);
         return;
     }
  } else if (argc == 1) {
      device = argv[0];
  } else {
      term_printf("bad number of options\n");
      return;
  }

  ...
}

Consider multiplying that by all of the possible monitor commands, and 
it's totally not worth it.

Regards,

Anthony Liguori

> Laurent
>
>   

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Laurent Vivier]

Le lundi 11 août 2008 à 20:57 -0500, Anthony Liguori a écrit :
> Laurent Vivier wrote:
> > Le lundi 11 août 2008 à 14:22 -0500, Anthony Liguori a écrit :
> >   
> >
> > but using "void (*handler)(int argc, char** argv)" avoids the switch:
> >
> > switch(nb_args) {
> >     case 0:
> >         cmd->handler();
> >         break;
> >     case 1:
> >         cmd->handler(args[0]);
> >         break;
> > ...
> > }
> >
> > becomes
> >
> > cmd->handler(nb_args, args);
> >   
> 
> And then every monitor command changes from:
> 
> void do_eject(int force, char *device)
> {
>      ...
> }
> 
> to:
> 
> void do_eject(int argc, char **argv)
> {
>    char *device;
>    int force = 0;
> 
>    if (argc == 2) {
>      if (strcmp(argv[0], "-f") == 0) {
>          force = 1;
>          device = argv[1];
>      } else {
>          term_printf("bad option %s\n", argv[0]);
>          return;
>      }
>   } else if (argc == 1) {
>       device = argv[0];
>   } else {
>       term_printf("bad number of options\n");
>       return;
>   }
> 
>   ...
> }

Yes

> Consider multiplying that by all of the possible monitor commands, and 
> it's totally not worth it.

it's cleaner and more flexible...
and from an artistic point of view, it's beautiful ;-)

Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Avi Kivity]

Anthony Liguori wrote:
> Laurent Vivier wrote:
>> Le lundi 11 août 2008 à 14:22 -0500, Anthony Liguori a écrit :
>>  
>> but using "void (*handler)(int argc, char** argv)" avoids the switch:
>>
>> switch(nb_args) {
>>     case 0:
>>         cmd->handler();
>>         break;
>>     case 1:
>>         cmd->handler(args[0]);
>>         break;
>> ...
>> }
>>
>> becomes
>>
>> cmd->handler(nb_args, args);
>>   
>
> And then every monitor command changes from:
>
> void do_eject(int force, char *device)
> {
>     ...
> }
>
> to:
>
> void do_eject(int argc, char **argv)
> {
>   char *device;
>   int force = 0;
>
>   if (argc == 2) {
>     if (strcmp(argv[0], "-f") == 0) {
>         force = 1;
>         device = argv[1];
>     } else {
>         term_printf("bad option %s\n", argv[0]);
>         return;
>     }
>  } else if (argc == 1) {
>      device = argv[0];
>  } else {
>      term_printf("bad number of options\n");
>      return;
>  }
>
>  ...
> }
>
> Consider multiplying that by all of the possible monitor commands, and 
> it's totally not worth it.
>


I forgot about non-string args.  So the transformation would be:

void do_eject(void **args)
{
    int *force = *(int *)args[0];
    const char *device = *(const char **)args[1];

   ...
}

But this isn't really an improvement, apart from dropping the ugly switch.

(maybe a union:

typedef union {
    int i;
    const char *s;
    ...
} Arg;

void do_eject(const Args *args)
{
    int force = args++->i;
    const char *device = args++->s;

    ...
}

)

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

Re: [Qemu-devel] [RFC, PATCH] Add -Wstrict-prototypes, maybe later -Wmissing-prototypes

[Daniel Jacobowitz]

On Mon, Aug 11, 2008 at 02:22:16PM -0500, Anthony Liguori wrote:
> But this tosses a warning since void (*)() is deprecated.  So, if we use:
>
> void dispatch(void *func, int n_args, void *args)
>
> It'll work just like it did before.

Just for the record: while you're correct this will work on mainstream
systems (you've been saying modern, which is not accurate), it is not
valid C.  The C standard only permits conversions between pointers to
different object types, or between pointers to different function
types.  Pointers can also be converted to integers but "the result
need not be in the range of values of any integer type".

GCC will even warn about it:
  ISO C forbids conversion of object pointer to function pointer type

-- 
Daniel Jacobowitz
CodeSourcery