Re: Pending breakpoints in MI

[Joel Brobecker <brobecker at adacore dot com>]

Volodya,

You forgot to provide a ChangeLog...

Here are my comments on your patch itself. It's mostly fine so we should
be able to check it in soon :).

>  gdb_breakpoint (char *address, char *condition,
>  		int hardwareflag, int tempflag,
>  		int thread, int ignore_count,
> +                int pending,
>  		char **error_message)

I can't believe I'm saying this, since I absolutely hate tabs, but
these are the coding rules: You need to use a tab instead of 8 spaces
in the line above. Compare for instance how the "char **error_message"
line is indented.

> -                                 AUTO_BOOLEAN_FALSE /* no pending. */,
> +                                 pending 
> +                                 ? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE,

Same here.

>  			    int thread, int ignore_count,
> +                            int pending,
>  			    char **error_message);

Likewise in the .h prototype.

> --- gdb/testsuite/gdb.mi/mi-pending.exp	(/patches/pending_mi_2_code_duplication)	(revision 45)
> +++ gdb/testsuite/gdb.mi/mi-pending.exp	(/patches/pending_mi_3_breakpoints)	(revision 45)
> @@ -0,0 +1,75 @@
> +# Copyright 2003, 2004, 2005, 2007 Free Software Foundation, Inc.

Shouldn't the copyright year be only 2007?

> --- gdb/testsuite/gdb.mi/mi-pending.c	(/patches/pending_mi_2_code_duplication)	(revision 45)
> +++ gdb/testsuite/gdb.mi/mi-pending.c	(/patches/pending_mi_3_breakpoints)	(revision 45)
> @@ -0,0 +1,34 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2004, 2007 Free Software Foundation, Inc.

Same question here.

> --- gdb/testsuite/gdb.mi/mi-pendshr.c	(/patches/pending_mi_2_code_duplication)	(revision 45)
> +++ gdb/testsuite/gdb.mi/mi-pendshr.c	(/patches/pending_mi_3_breakpoints)	(revision 45)
> @@ -0,0 +1,32 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2004, 2007 Free Software Foundation, Inc.

Likewise here...

> --- gdb/mi/mi-cmd-break.c	(/patches/pending_mi_2_code_duplication)	(revision 45)
> +++ gdb/mi/mi-cmd-break.c	(/patches/pending_mi_3_breakpoints)	(revision 45)
> @@ -63,7 +63,10 @@ enum bp_type
>     breakpoint.  -break-insert -t -h <location> --> insert a temporary
>     hw bp.  
>     -break-insert -r <regexp> --> insert a bp at functions matching
> -   <regexp> */
> +   <regexp> 
> +
> +   The -f flag makes GDB create a pending breakpoint if no
> +   locations for breakpoint are found now.  */

Actually, I think that the entire comment needs to be rewritten
in a way that the syntax is not described there at all. Just say
that this function implements the -break-insert command which, as
its name suggests, inserts a breakpoint.  The syntax needs to be
documented in the GDB manual. See
http://www.sourceware.org/gdb/current/onlinedocs/gdb_25.html#SEC259
for what is currently documented. Let's use this opportunity to
make sure that every option (and only the supported options) are
documented.

> @@ -122,6 +127,9 @@ mi_cmd_break_insert (char *command, char
>  	case THREAD_OPT:
>  	  thread = atol (optarg);
>  	  break;
> +        case PENDING_OPT:
> +          pending = 1;
> +          break;

The indentation is strange, could you double-check it?

> @@ -139,12 +147,14 @@ mi_cmd_break_insert (char *command, char
>        rc = gdb_breakpoint (address, condition,
>  			   0 /*hardwareflag */ , temp_p,
>  			   thread, ignore_count,
> +                           pending,
>  			   &mi_error_message);
>        break;
>      case HW_BP:
>        rc = gdb_breakpoint (address, condition,
>  			   1 /*hardwareflag */ , temp_p,
>  			   thread, ignore_count,
> +                           pending,

I think you need to replace some spaces with tabs here too...

Thanks,
-- 
Joel