Re: [for-5.0 PATCH v2 1/3] cpu: Introduce CPUReset callback typedef

[Greg Kurz <groug@kaod.org>]

On Tue, 10 Dec 2019 10:42:51 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Tue, 10 Dec 2019 at 10:39, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Greg Kurz <groug@kaod.org> writes:
> > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > > index 77c6f0529903..047e3972ecaf 100644
> > > --- a/include/hw/core/cpu.h
> > > +++ b/include/hw/core/cpu.h
> > > @@ -74,6 +74,8 @@ typedef struct CPUWatchpoint CPUWatchpoint;
> > >
> > >  struct TranslationBlock;
> > >
> > > +typedef void (*CPUReset)(CPUState *cpu);
> > > +
> > >  /**
> > >   * CPUClass:
> > >   * @class_by_name: Callback to map -cpu command line model name to an
> > > @@ -165,7 +167,7 @@ typedef struct CPUClass {
> > >      ObjectClass *(*class_by_name)(const char *cpu_model);
> > >      void (*parse_features)(const char *typename, char *str, Error **errp);
> > >
> > > -    void (*reset)(CPUState *cpu);
> > > +    CPUReset reset;
> > >      int reset_dump_flags;
> > >      bool (*has_work)(CPUState *cpu);
> > >      void (*do_interrupt)(CPUState *cpu);
> > [...]
> >
> > Opinion, not objection: such typedefs make the code less obvious.
> 
> It's particularly odd here where this class has several
> methods but we've only chosen one to privilege with a typedef.
> 

Yes, children classes don't do the overloading-and-call-the-parent for
other methods which was the initial motivation for the typedef.

> Personal preference: if you use a typedef, typedef the
> function type, not the pointer-to-the-function-type.
> But I would just leave it be.
> 

Thinking again, I'm not sure the typedef really helps here. Markus
doesn't like it either. I'll try without.

> thanks
> -- PMM