From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7604 invoked by alias); 30 Aug 2011 12:45:59 -0000 Received: (qmail 7592 invoked by uid 22791); 30 Aug 2011 12:45:57 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 30 Aug 2011 12:45:38 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p7UCjbf8010832 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 30 Aug 2011 08:45:37 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p7UCjZbM028031; Tue, 30 Aug 2011 08:45:36 -0400 From: Phil Muldoon To: matt rice Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/7] [python] API for macros: Add gdb.Macro class. References: <1314198654-9008-1-git-send-email-ratmice@gmail.com> <1314198654-9008-4-git-send-email-ratmice@gmail.com> Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Tue, 30 Aug 2011 12:45:00 -0000 In-Reply-To: <1314198654-9008-4-git-send-email-ratmice@gmail.com> (matt rice's message of "Wed, 24 Aug 2011 08:10:50 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00595.txt.bz2 matt rice 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, ¯o_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, ¯o_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, ¯o_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