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

Pedro Alves
2016-10-27T14:54:56Z

On 08/31/2016 04:05 PM, Yao Qi wrote:
> This patch adds an enum mips_breakpoint_kinds to avoid using magic
> numbers as much as possible.
>
> gdb:
>
> 2016-08-31 Yao Qi <yao.qi@linaro.org>
>
> * mips-tdep.c (mips_breakpoint_kinds): New enum.
> (mips_breakpoint_from_pc): Use it.
> (mips_remote_breakpoint_from_pc): Likewise.
> ---
> gdb/mips-tdep.c | 37 ++++++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 4e4d79e..34df8d0 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -107,6 +107,20 @@ static const char *const mips_abi_strings[] = {
> NULL
> };
>
> +/* 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;

> +{
> + /* 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.

> +};
> +
> /* For backwards compatibility we default to MIPS16. This flag is
> overridden as soon as unambiguous ELF file flags tell us the
> compressed ISA encoding used. */
> @@ -7143,16 +7157,7 @@ mips_breakpoint_from_pc (struct gdbarch *gdbarch,
> }
> }
>
> -/* Determine the remote breakpoint kind suitable for the PC. The following
> - kinds are used:
> -
> - * 2 -- 16-bit MIPS16 mode breakpoint,
> -
> - * 3 -- 16-bit microMIPS mode breakpoint,
> -
> - * 4 -- 32-bit standard MIPS mode breakpoint,
> -
> - * 5 -- 32-bit microMIPS mode breakpoint. */

In a latter patch you seem to read this comment.

> +/* Determine the remote breakpoint kind suitable for the PC. */

Thanks,
Pedro Alves