From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22749 invoked by alias); 21 Jan 2013 17:09:37 -0000 Received: (qmail 22734 invoked by uid 22791); 21 Jan 2013 17:09:35 -0000 X-SWARE-Spam-Status: No, hits=-6.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,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; Mon, 21 Jan 2013 17:09:24 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r0LH9NmR019141 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 21 Jan 2013 12:09:23 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r0LH9L8f015038 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 21 Jan 2013 12:09:22 -0500 From: Tom Tromey To: Siva Chandra Cc: gdb-patches@sourceware.org Subject: Re: [RFC - GDB Python API] New gdb.Architecture class References: Date: Mon, 21 Jan 2013 17:09:00 -0000 In-Reply-To: (Siva Chandra's message of "Mon, 21 Jan 2013 06:08:14 -0800") Message-ID: <87sj5u31ku.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2.91 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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: 2013-01/txt/msg00480.txt.bz2 >>>>> "Siva" == Siva Chandra writes: Siva> http://sourceware.org/ml/gdb-patches/2013-01/msg00296.html, the Siva> attached patch adds a new class 'gdb.Architecture' with a single Siva> method 'name'. I will add tests and docs after we are OK with the code Siva> changes. I think the basic idea is fine. I found a few nits in the patch, nothing too serious. Siva> +struct gdbarch * Siva> +arch_object_to_gdbarch (PyObject *obj) Siva> +{ Siva> + arch_object *py_arch = (arch_object *) obj; Siva> + return py_arch->gdbarch; Missing newline after declaration. Siva> +PyObject * Siva> +gdbarch_to_arch_object (struct gdbarch *gdbarch) Siva> +{ Siva> + arch_object *obj = PyObject_New (arch_object, &arch_object_type); Siva> + if (obj == NULL) Siva> + { Siva> + PyErr_SetString (PyExc_MemoryError, Siva> + _("Could not allocate architectire object")); Siva> + return NULL; Siva> + } Siva> + Siva> + obj->gdbarch = gdbarch; Siva> + return (PyObject *) obj; Siva> +} Likewise. Also, I think there's no need to call PyErr_SetString here. Just return NULL; PyObject_New will have already set the error appropriately. One other question is whether we care about object identity. gdbarch_to_arch_object returns a new object each time. Either way is fine depending on circumstances -- we do both already in gdb -- but it ought to be an explicit choice. Siva> +static PyObject * Siva> +archpy_name (PyObject *self, PyObject *args) { Wrong brace placement. Siva> + PyObject *py_name = Py_BuildValue ("s", name); Siva> + Siva> + return py_name; Just use PyString_FromString here. Siva> +PyObject * Siva> +gdbpy_current_arch (PyObject *self, PyObject *args) { Wrong brace placement. Do we need this function? The "current arch" is kind of a problematic feature. It is semi-exposed to Python right now, but I think we're mostly agreed that this was an early mistake. I think the more desirable approach would be to have arch methods on the appropriate objects -- Frame, but also whatever else, say maybe objfile or inferior (I'm not really all that sure...). Anyway, if you do need it: Siva> + struct gdbarch *curr_arch = get_current_arch (); .. then why get_current_arch and not 'python_gdbarch'? Siva> +static PyObject * Siva> +frapy_arch (PyObject *self, PyObject *args) Siva> +{ Siva> + struct frame_info *frame = NULL; /* Initialize to appease gcc warning. */ Siva> + frame_object *obj = (frame_object *) self; Siva> + volatile struct gdb_exception except; Siva> + enum unwind_stop_reason stop_reason; I didn't see a use of stop_reason. Siva> +extern PyTypeObject arch_object_type; This didn't seem to be used outside of the new file, so it could just be static there. Tom