Re: [python] [rfc] Patch for pretty-printers registration API change

[Tom Tromey <tromey at redhat dot com>]

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

Phil> Please do let me know  your comments and thoughts on this change. I
Phil> look forward to hearing them.

This is looking pretty good.

I have lots of little nits.  Don't let them get you down, the most
important parts of this patch are ok.

Phil> +The Python list @code{gdb.pretty_printers} contains an array of
Phil> +functions that have been registered via addition as a
Phil> +pretty-printer. Each function will be called with a @code{gdb.Value}

GNU follows the American style and puts two spaces after a period.

Phil> +to be pretty-printed.  Each @code{gdb.Objfile} also contains a
Phil> +@code{pretty_printers} attribute. In both contexts, these functions
Phil> +takes a single @code{gdb.Value} argument and returns the output of the
Phil> +pretty-printer conforming to the interface definition above, or if the
Phil> +pretty-printer cannot print the value, returns the Python value: None.

This sentence reads strangely.  How about:

  A function on one of these lists takes a single @code{gdb.Value}
  argument and returns a pretty-printer object conforming to the
  interface definition above.  If this function cannot create a
  pretty-printer for the value, it should return @code{None}.

Phil> +@code{gdb.Objfile} and iteratively calls each function in the list for
Phil> +that @code{gdb.Objfile} until it receives pretty-printed output.

I think:  ... until it receives a pretty-printer object.

Phil> +After these @code{gdb.Objfile} have been exhausted, it tries the global
Phil> +@code{gdb.pretty-printers} list, again calling each function until
Phil> +output is generated.
 
"until an object is returned".

Phil> +Similarly, the order in which the functions are called in each list
Phil> +is not specified.
 
I think we do want to specify the order in the list case.  We should
always invoke the functions starting from the head of the list.  With
dictionaries there is no natural ordering; but with lists there is.

Phil> +How printers are selected is defined by the user. @value{GDBN}
Phil> +provides the value, and from this a decision returned whether the
Phil> +printer can print the value.

The grammar is weird here.  Also, avoid the passive voice, upstream
will ding you for this.

Phil> +the type. If it is a type the printer is interested in, return an
Phil> +instance of the printer. If not, return: None.

"return @code{None}"

Phil> +    if (lookup_tag  == None):

No parens here.  Also there is an extra space before the ==.
I know this is just an example in the docs but I think we want to
exhibit typical Python style.

Phil> -        self.n_buckets = hash['_M_bucket_count']
Phil> +        self.n_buckets = hash['_M_element_count']

I think we ought to leave this hunk out and then put it back in a
later patch -- one that fixes all the bugs in this iterator.

Phil> +    # If it points to a reference, get the reference

In the GNU style, a comment must be a full sentence, starting with a
capital letter and ending with a period.  This one misses the period,
but others have other problems.

Phil> +    # Get the unqualified type, stripped of type defs

Just "typedefs".  Also the period.

Phil> +    # Iterate over local dictionary of types to determine
Phil> +    # if a printer is registered for that type. Return an

Two spaces after a period in comments, too.

Phil> +/* Return the type, stripped of TypeDefs */

Just "typedefs".  And the elusive period.

Phil> +static PyObject *
Phil> +typy_strip_typedefs (PyObject *self, PyObject *args)
[...]
Phil> +  return type_to_type_object (CHECK_TYPEDEF (type));

CHECK_TYPEDEF is a macro typically called for side effects.
Here it is better to call check_typedef.

Phil> +search_pp_list (PyObject *list, struct value *value)

By taking a struct value, this function creates a new gdb.Value for
each printer.  If the printer returns None (which will be the most
common case), then the new Value is immediately destroyed.

Instead, I think it would be better to instantiate the Value once,
before looping over all the printers.  This means updating this to
take a PyObject and also updating gdbpy_instantiate_printer.

Phil> +      function = PyList_GetItem (list, list_index);

Needs a check for failure.

Phil> +      if (PyErr_Occurred ())
Phil> +	gdbpy_print_stack ();

I suspect that we should just propagate errors that occur while
calling functions from the list.  My thinking is that any error here
represents a bug in some Python code, and we shouldn't proceed past
that point.

However, I'm not certain about this.

Phil> +      if ((printer) && (printer != Py_None))

Too many parens.

Phil> +  return Py_None;

The function's introductory comment claims that it returns a new
reference.  So, you need Py_RETURN_NONE here.

Phil> +find_pretty_printer (struct value *value)
[...]

Phil> +  if (! PyList_Check (pp_list) || ! PyList_Size (pp_list))

I don't think we need the PyList_Size check here.
If it is zero, search_pp_list will just return.
 
Phil> +  if (! function)
Phil> +      function = Py_None;

Indentation looks wrong on the second line.

Phil> +  Py_INCREF (function);
 
If search_pp_list does return a new reference, then this is not needed.
Instead it has to be hoisted into the 'function = Py_None' line, above.

Phil> @@ -1174,6 +1158,8 @@ print_children (PyObject *printer, const char *hint,
 
Phil>        if (! item)
Phil>  	{
Phil> +	  if (PyErr_Occurred ())
Phil> +	    gdbpy_print_stack ();
Phil>  	  /* Set a flag so we can know whether we printed all the
Phil>  	     available elements.  */
Phil>  	  done_flag = 1;

I am not sure but I suspect this should be:

   if (PyErr_Occurred ())
     gdbpy_print_stack ();
   else
     {
        ...
     }

IOW, do we want to set done_flag if the iterator failed?

Maybe it doesn't really matter.

Phil> +  return cons      ;

Excess spaces.

Phil> +  if (var->value)
Phil> +      pretty_printer = gdbpy_get_varobj_pretty_printer (var->value);

The second line looks it is indented too far.

Phil> +  Py_DECREF(constructor);

Missing space before the "(".

thanks,
Tom