Re: [patch] Add visible flag to breakpoints.

[Tom Tromey <tromey@redhat.com>]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> In this patch iteration I removed any form of storage/tracking of
Phil> breakpoints.  Instead, as above, I placed a reference in the breakpoint
Phil> struct. 

I like this better.

Phil> There are two disadvantages that stem from this:
Phil> * I had to extern breakpoint_chain
Phil> * I had to move the ALL_BREAKPOINTS macro to breakpoint.h
Phil> I'm not sure how to get around those, or, if indeed they are perceived
Phil> as disadvantages.

It is definitely bad to do this.  I think it is fixable, though.

It is a little odd that the observer passes the number and not the
breakpoint itself.

Phil> +  if (internal && PyObject_IsTrue (internal))
Phil> +    internal_bp = 1;

PyObject_IsTrue can return -1 on failure, so this code must account for
that.

I see we have a case where we don't check this in py-prettyprint.c :(
I will fix that.

Phil> +      int i = 0;
Phil> +      ALL_BREAKPOINTS (b)

This instance of ALL_BREAKPOINTS should probably be replaced with some
kind of callback API, like the other iterate_over_* functions in gdb.

Phil> +      {
Phil> +	/* Not all breakpoints will have a companion Python object.
Phil> +	   Only breakpoints that were created via bppy_new, or
Phil> +	   breakpoints that were created externally and are tracked by
Phil> +	   the Python Scripting API.  */
Phil> +	if (b->py_bp_object)
Phil> +	  {
Phil> +	    if (PyTuple_SetItem (result, i, (PyObject *) b->py_bp_object) != 0)

I think that cast to PyObject is unnecessary.

Phil> +	      {
Phil> +		Py_DECREF (result);
Phil> +		return NULL;
Phil> +	      }
Phil> +	    Py_INCREF (b->py_bp_object);

This incref should come before the call to PyTuple_SetItem.

Phil> @@ -667,9 +673,7 @@ gdbpy_breakpoint_created (int num)
Phil>    breakpoint_object *newbp;
Phil>    struct breakpoint *bp = NULL;
Phil>    PyGILState_STATE state;
Phil> -
Phil> -  if (num < 0)
Phil> -    return;
Phil> +  int error = 0;

I am not certain that we want a new breakpoint object to be created for
internal breakpoints set by other modules.  It seems potentially
harmful.

I think one way to do this would be to rephrase this check as

    if (num < 0 && bppy_pending_object == NULL)

Phil> +      newbp->bp->py_bp_object = (PyObject *)newbp;

No cast.

Phil> +  if (! error)
Phil> +    ++bppy_live;

You can get rid of the 'error' local and just push this back up into the
"ok" branch of the preceding 'if'.
 
Phil>    /* Just ignore errors here.  */
Phil>    PyErr_Clear ();

I think this is redundant now.

Phil> +  ALL_BREAKPOINTS (b)
Phil> +  {
Phil> +    if (b->number == num)
Phil> +      {
Phil> +	breakpoint_object *bp_obj = 
Phil> +	  ((breakpoint_object *)b->py_bp_object);

I think you can replace this ALL_BREAKPOINTS with a call to
get_breakpoint.

Space after the close paren in the cast.

Tom