This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 06/13] Add enum for mips breakpoint kinds
- From: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: Pedro Alves <palves at redhat dot com>, Yao Qi <qiyaoltc at gmail dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Fri, 28 Oct 2016 20:39:46 +0100
- Subject: Re: [PATCH 06/13] Add enum for mips breakpoint kinds
- Authentication-results: sourceware.org; auth=none
- References: <1472655965-12212-1-git-send-email-yao.qi@linaro.org> <1472655965-12212-7-git-send-email-yao.qi@linaro.org> <781ba8eb-ba42-58e0-1ead-ae44d8a39409@redhat.com>
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