From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12745 invoked by alias); 4 Jul 2011 09:13:39 -0000 Received: (qmail 12555 invoked by uid 22791); 4 Jul 2011 09:13:38 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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, 04 Jul 2011 09:13:10 +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 p649D8ha017791 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 4 Jul 2011 05:13:08 -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 p649D7od009708; Mon, 4 Jul 2011 05:13:07 -0400 From: Phil Muldoon To: Matt Rice Cc: gdb-patches@sourceware.org Subject: Re: [patch] [python] find_line_pc_range References: Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Mon, 04 Jul 2011 10:09:00 -0000 In-Reply-To: (Matt Rice's message of "Sun, 3 Jul 2011 23:03:51 -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-07/txt/msg00083.txt.bz2 Matt Rice writes: > Thanks, attached is an updated patch that also includes tests. Thanks. > + ret_tuple = PyTuple_New (2); > + if (!ret_tuple) > + return Py_None; Return NULL here. This signifies an error, and that we have abandoned the function. When a Python function returns NULL, the exception is eventually printed and cleared either by the callers, or by the GDB Python top-level exception handling code. > + start = gdb_py_long_from_ulongest (start_pc); > + if (!start) > + goto fail_ret; > + if (PyTuple_SetItem (ret_tuple, 0, start) != 0) > + { > + Py_XDECREF (start); > + goto fail_ret; > + } You do not need to decrement "start" here. Even if the tuple SetItem call failed, you have already passed the responsibility to the tuple, and you no longer own it (stolen reference). SetItem would decrement this on failure. So just goto to your local error block. > + end = gdb_py_long_from_ulongest (end_pc); > + if (!end) > + goto fail_ret; > + if (PyTuple_SetItem (ret_tuple, 1, end) != 0) > + { > + Py_XDECREF (end); > + goto fail_ret; > + } Same as above. > + fail_ret: > + Py_XDECREF (ret_tuple); Small nit, and this is generally ok, but you already know that if this error block is called, ret_tuple will be instantiated. So in this case the XDECREF is a tiny bit redundant. You could just use DECREF here and skip the NULL check that XDECREF does. Like I said, a tiny nit, but I just thought I would point it out. > + return Py_None; If there is a Python exception that you cannot handle internally to your function, you must always return NULL. This informs callers, and ultimately the exception managing code in GDB that some Python call failed, to print the exception, and clear it. > @@ -526,6 +573,8 @@ static PyMethodDef sal_object_methods[] = { > { "is_valid", salpy_is_valid, METH_NOARGS, > "is_valid () -> Boolean.\n\ > Return true if this symbol table and line is valid, false if not." }, > + { "find_line_pc_range", salpy_find_line_pc_range, METH_NOARGS, > + "Return a tuple of start and end ranges for the symbol table." }, > {NULL} /* Sentinel */ > }; We try to put the return type in the help now as a formal statement. Look at the two-line help example above: "is_valid()" for guidance. Nearly there, thanks for the patch! Cheers, Phil