Re: [PATCH 06/13] Add enum for mips breakpoint kinds

Maciej W. Rozycki
2016-10-28T19:39:58Z

Pedro, Yao --

I missed the MIPS parts previously, sorry.

On Thu, 27 Oct 2016, Pedro Alves wrote:

> > +/* Enum describing the different kinds of breakpoints. */
> > +
> > +enum mips_breakpoint_kinds
>
> IMO that should be singular. Imagine you put one of these
> in a variable. Like:
>
> enum mips_breakpoint_kinds kind;
>
> This would be more natural, IMO:
>
> enum mips_breakpoint_kind kind;

Agreed.

> > +{
> > + /* 16-bit MIPS16 mode breakpoint */
> > + MIPS_BP_KIND_16BIT_MIPS16 = 2,
> > + /* 16-bit microMIPS mode breakpoint */
> > + MIPS_BP_KIND_16BIT_MICROMIPS = 3,
> > + /* 32-bit standard MIPS mode breakpoint */
> > + MIPS_BP_KIND_32BIT = 4,
> > + /* 32-bit microMIPS mode breakpoint */
> > + MIPS_BP_KIND_32BIT_MICROMIPS = 5,
>
> IMO a line break between these makes it much more readable.
>
> /* 16-bit MIPS16 mode breakpoint */
> MIPS_BP_KIND_16BIT_MIPS16 = 2,
>
> /* 16-bit microMIPS mode breakpoint */
> MIPS_BP_KIND_16BIT_MICROMIPS = 3,
>
> etc.

Indeed. Also a full stop and two spaces at the end are due.

Also how about calling them:

MIPS_BP_KIND_MIPS16
MIPS_BP_KIND_MICROMIPS16
MIPS_BP_KIND_MIPS32
MIPS_BP_KIND_MICROMIPS32

to make the names a bit shorter though still retaining the meaning?

Maciej