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

Phil Muldoon
2011-08-30T12:45:59Z

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