Re: [Qemu-devel] Re: [PATCH 03/12] Refactor and enhance break/watchpoint API

[Jamie Lokier <jamie@shareable.org>]

Jan Kiszka wrote:
> Anthony Liguori wrote:
> > Jan Kiszka wrote:
> >> Index: b/gdbstub.c
> >> ===================================================================
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -1145,10 +1145,64 @@ void gdb_register_coprocessor(CPUState *
> >>      }
> >>  }
> >>  
> >> +/* GDB breakpoint/watchpoint types */
> >> +#define GDB_BREAKPOINT_SW        0
> >> +#define GDB_BREAKPOINT_HW        1
> >> +#define GDB_WATCHPOINT_WRITE     2
> >> +#define GDB_WATCHPOINT_READ      3
> >> +#define GDB_WATCHPOINT_ACCESS    4
> >> +
> >> +#ifndef CONFIG_USER_ONLY
> >> +static const int xlat_gdb_type[] = {
> >> +    [GDB_WATCHPOINT_WRITE]  = BP_GDB | BP_MEM_WRITE,
> >> +    [GDB_WATCHPOINT_READ]   = BP_GDB | BP_MEM_READ,
> >> +    [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
> >> +};
> >> +#endif
> >> +
> >> +static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
> >> +                                 target_ulong len, int type)
> >> +{
> >> +    switch (type) {
> >> +    case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
> >>   
> > 
> > We've avoided this GCCism pretty much.  I don't think the code is
> > significantly cleaner with it so I think it's best to avoid it.
> 
> OK, I see the general problem. Restricting ourselves here is not a big
> issue - but for my other series which tried hard to canonicalizes x86's
> cpu_gdb_read/write_register, sigh...

I think the code would be clearer with separate cases in this instance anyway.

Usuallywhen you have an enumeration which is just labels without a
numerical purpose, seeing "..." or "<" on it is not clear which cases
are included, and it tends to break quietly when someone adds more values.

If there's a long sequence of cases which crops up often, then you can
use a macro "#define cases_FOO case FOO1: case FOO2: case FOO3:".

-- Jamie