Re: [PATCH 3/7] [python] API for macros: Add gdb.Macro class.

[Phil Muldoon <pmuldoon@redhat.com>]

matt rice <ratmice@gmail.com> writes:

> +#include "defs.h"
> +#include "python-internal.h"
> +#include "py-macro.h"
> +#include "macrotab.h"
> +#include "bcache.h"
> +#include "objfiles.h"
> +
> +/* The macro_object_validator type has the ability to detect when an object
> +   has been invalidated and nothing more.  The more featured validation
> +   methods of pyst, pysal, provide no benefit for macros because once
> +   we have been invalidated, we have no way to know what macro the object
> +   once represented.  */
> +typedef struct
> +{
> +  PyObject_HEAD;
> +  int is_valid;
> +  struct objfile *objfile;
> +} macro_validator_object;

^^ Comments on each member, please.

> +typedef struct
> +{
> +  PyObject_HEAD;
> +
> +  /* A macro definition object representing this macro.  */
> +  const struct macro_definition *def;
> +  /* The name of the macro */
> +  const char *name;
> +  /* The file where the macro resides and its include trail.  */
> +  struct macro_source_file *src;
> +  /* the line location in the 'SRC' file.  */

Capital 'T'.

> +  int line;
> +  /* This is used to tell us if the macro has been invalidated

Missing period.

> +     The objfile who's obstack and bcache the mdef, and src, and name
> +     fields reside in is stored here.  Those fields may be dangling
> +     pointers if the objfile is NULL.
> +     An invalid macro cannot become valid once again.  */
> +  macro_validator_object *objfile_validator;
> +} macro_object;

I thought you removed the bcache?

> +macropy__definition_to_py (const struct macro_definition *macro)

A small nit for me personally, but all of the other Python API classes
use one "_" to separate the function library name from the function
name.  This and others.


> +macropy_object_create (struct objfile *objfile,
> +		       const char *name,
> +		       const struct macro_definition *def,
> +		       struct macro_source_file *src,
> +		       int line)
> +{
> +  macro_object *macro_obj;
> +
> +  macro_obj = PyObject_New (macro_object, &macro_object_type);
> +
> +  if (macro_obj)
> +    {
> +      macro_validator_object *validator;
> +
> +      macro_obj->objfile_validator = NULL;
> +
> +      /* check our arguments, Don't check line because 0 is a valid line.  */

Capital "C". "." instead of ",".


> +macropy__validate_self (PyObject *self)
> +{
> +  macro_object *me;
> +
> +  if (! PyObject_TypeCheck (self, &macro_object_type))
> +    {
> +      PyErr_SetString (PyExc_RuntimeError,
> +			 _("Typecheck failure converting macro."));
> +      return NULL;
> +    }
> +
> +  me = (macro_object *) self;
> +
> +  if (me->objfile_validator->is_valid == 0)
> +    {
> +      PyErr_SetString (PyExc_RuntimeError, _("Macro is invalid."));
> +      return NULL;
> +    }
> +
> +  return me;
> +}

As you are pointing to an existing copy of self, and returning it, you
*may* need to incref.  It depends if any of the functions downstream
decref it.  Please check.

> +static int
> +macropy_compare (PyObject *self, PyObject *o2)
> +{
> +  PyObject *my_str = macropy_str (self);
> +  PyObject *other_str = NULL;
> +  PyObject *an_error;
> +  int err_code = -1;
> +  int comparison_result = -1;
> +
> +  if (!my_str)
> +    goto check_for_errors_and_return;
> +
> +  if (PyObject_TypeCheck (o2, &macro_object_type))
> +    {
> +      other_str = macropy_str (o2);
> +      if (!other_str)
> +	goto check_for_errors_and_return;
> +
> +      err_code = PyObject_Cmp (my_str, other_str, &comparison_result);
> +    }

Use PyObject_Compare

> +  else
> +    {
> +      err_code = PyObject_Cmp (my_str, o2, &comparison_result);
> +    }

Ditto.

> +  check_for_errors_and_return:
> +    Py_XDECREF (my_str);
> +    Py_XDECREF (other_str);
> +
> +    /* Because -1 is also Less Than we cannot use gdbpy_print_stack ()
> +       which clears the error if set python print-stack is off.
> +       Which would lead to (gdb) python print x == x
> +       returning False with no error message displayed.
> +
> +       alternately if an error is set and the return value is not 1,
> +       python will complain.
> +
> +       Thus if there is an error, we must normalize it
> +       making sure that both an error and the return
> +       value are -1. Should we encounter one.  */
> +    an_error = PyErr_Occurred ();
> +    if (an_error == NULL && err_code != -1)
> +      {
> +        /* a valid comparison */
> +        return comparison_result;
> +      }
> +    else if (an_error == NULL && err_code == -1)
> +      {
> +	/* an unknown error */
> +	PyErr_SetString (PyExc_RuntimeError,
> +			 _("mystery error during macro comparison."));


Don't overwrite the exception here.  And if PyObject_Compare has
returned -1, there will be an exception so this can't happen.  You could
probably just remove the condition entirely and just fold over to the
else below.

> +        return -1;
> +      }
> +    else /* a known error */
> +      return -1;
> +}
> +


> +static long
> +macropy_hash (PyObject *o)
> +{
> +  long result = -1;
> +  PyObject *my_str = macropy_str (o);
> +
> +  if (my_str)
> +    result = PyObject_Hash (my_str);
> +
> +  /* The docs say PyObject_Hash should return -1 on error,
> +     Don't believe it because it is not always true,
> +     The exception gets raised regardless of return value,
> +     But we should follow the documented return strategy of -1 on error.

Even if you are right, are you right for all versions of Python?  I
guess the PyErr_Occurred check is harmless.

> +static PyMethodDef macro_object_methods[] = {
> +  { "argv", macropy_argv_method, METH_NOARGS,
> +    "argv () -> List.\n\
> +Return a list containing the names of the arguments for a function like \
> +macro." },

"function-like", imo.

> +  { "definition", macropy_definition_method, METH_NOARGS,
> +    "definition () -> String.\n\
> +Return the macro's definition." },
> +  { "include_trail", macropy_include_trail_method, METH_NOARGS,
> +    "include_trail () -> List.\n\
> +Return a list containing tuples with the filename and line number describing \
> +how and where this macro came to be defined." },

Your help and your return explanation don't match.  It should return a
Tuple (if it doesn't, please fix it).  And please change  -> List to ->
Tuple.


> +  struct objfile *objfile;
> +};
> +
> +/* A macro_callback_fn for use with macro_foreach and friends that adds
> +   a gdb.Macro object to the python list USER_DATA->LIST.
> +   USER_DATA should be of type struct macropy_user_data.
> +
> +   A null USER_DATA->OBJFILE is currently be considered an error,
> +   this is subject to change.
> +
> +   Aborts the iteration on error, and forces the macro iterator function
> +   to return macro_walk_aborted check the iterators macro_walk_result.
> +   On error it is the callers responsibility to dispose of the USER_DATA
> +   structure and its contents.  The appropriate python exception
> +   that caused the error has already been set.
> +*/
> +enum macro_walk_status
> +pymacro_add_macro_to_list (const char *name,
> +		   const struct macro_definition *definition,
> +		   struct macro_source_file *source,
> +		   int line,
> +		   void *user_data);
> +
> +/* Create a new macro (gdb.Macro) object that encapsulates
> +   the gdb macro representation.  OBJFILE is the objfile that contains
> +   the macro table.  NAME is the name of the macro.  DEF the macros definition
> +   SRC the source file containing the macro. LINE definitions location.  */
> +PyObject *
> +macropy_object_create (struct objfile *objfile,
> +		       const char *name,
> +		       const struct macro_definition *def,
> +		       struct macro_source_file *src,
> +		       int line);
> +
> +#endif /* GDB_PY_MACRO_H */

Any reason why these need their own include?  (over python-internal.h).
If these are called from anywhere other than python/* then they need to
go into python-internal.h to protect versions of GDB that do not have
Python scripting enabled.  (python.h conditionalizes this).


Cheers,

Phil