Re: [PATCH] Make tracepoints into breakpoints

[Joel Brobecker <brobecker at adacore dot com>]

Hi Stan,

I was scanning your patch a little, to see waht you had needed to do
in order to do this transition. I noticed a few things - not sure how
important they are, given how quickly I looked at it. But there you go.

> *************** bpstat_what (bpstat bs)
> *** 3324,3329 ****
> --- 3358,3367 ----
>   	  bs_class = bp_silent;
>   	  retval.call_dummy = 1;
>   	  break;
> + 
> + 	case bp_tracepoint:
> + 	  /* (should never occur, complain about it) */
> + 	  continue;
>   	}
>         current_action = table[(int) bs_class][(int) current_action];
>       }

This hunk looks really strange... Out of place?
The thing that caught my attention too was the comment that seem
to imply that this is a temporary comment, almost a FIXME. Not sure
if this is the case, but the complaint is not there (elsewhere maybe?).

> + /* Remove a tracepoint (or all if no argument) */

Nit-picking on the missing period at the end of your sentence. I just
happened to notice it, but I haven't really checked the rest.

> + #if 0 /* names of commands not visible here, not clear if worth fixing */
> + 		if (cmd_cfunc_eq (cmd, while_stepping_pseudocommand))
> + 		  indent = i2;
> + 		else if (cmd_cfunc_eq (cmd, end_actions_pseudocommand))
> + 		  indent = i1;
> + #endif

Do we really want to keep this #if 0? I'd personally rather have
a comment.

I'd also like to look at your patch again sometime soon, to figure out
whether it'd make sense or not for tracepoints to have their own
breakpoint_ops routines... To be continued :)

-- 
Joel