From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25756 invoked by alias); 19 Nov 2012 16:49:09 -0000 Received: (qmail 25589 invoked by uid 22791); 19 Nov 2012 16:49:04 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS 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, 19 Nov 2012 16:48:29 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qAJGmKEL009933 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 19 Nov 2012 11:48:21 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qAJGmJFg003879 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 19 Nov 2012 11:48:20 -0500 From: Tom Tromey To: Cc: Subject: Re: [PATCH] Python 3 support, part 1 (non-testsuite part) References: <87mwyma6xi.fsf@fleche.redhat.com> <87390d8hdf.fsf@fleche.redhat.com> <87y5i571xi.fsf@fleche.redhat.com> <87mwyl70hu.fsf@fleche.redhat.com> <87ehjx6yk1.fsf@fleche.redhat.com> Date: Mon, 19 Nov 2012 16:49:00 -0000 In-Reply-To: (Paul Koning's message of "Wed, 14 Nov 2012 16:03:22 +0000") Message-ID: <87ip91r12k.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (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: 2012-11/txt/msg00510.txt.bz2 >>>>> "Paul" == writes: Paul> Here is an updated patch which I believe addresses all the comments. Thanks. Paul> Should I add a NEWS entry for the Python 3 support? Yes, that would be good. Paul> PyTypeObject breakpoint_object_type = Paul> { Paul> - PyObject_HEAD_INIT (NULL) Paul> - 0, /*ob_size*/ Paul> + PyVarObject_HEAD_INIT (NULL, 0) The new line has the wrong indentation. Paul> static PyTypeObject cmdpy_object_type = Paul> { Paul> - PyObject_HEAD_INIT (NULL) Paul> - 0, /*ob_size*/ Paul> + PyVarObject_HEAD_INIT (NULL, 0) Ditto. Paul> PyTypeObject event_object_type = Paul> { Paul> - PyObject_HEAD_INIT (NULL) Paul> - 0, /* ob_size */ Paul> + PyVarObject_HEAD_INIT (NULL, 0) Ditto. Paul> static PyTypeObject eventregistry_object_type = Paul> { Paul> - PyObject_HEAD_INIT (NULL) Paul> - 0, /* ob_size */ Paul> + PyVarObject_HEAD_INIT (NULL, 0) Ditto. Paul> + if (except.reason < 0) Paul> + PyBuffer_Release (&pybuf); Paul> GDB_PY_HANDLE_EXCEPTION (except); Paul> +#ifdef IS_PY3K Paul> + PyBuffer_Release (&pybuf); Paul> +#endif I think the first PyBuffer_Release also has to be conditional on IS_PY3K here. It seems like you could just always call it after the try-catch rather than checking except.reason. Paul> +#ifdef IS_PY3K Paul> +static int Blank line between these two. Paul> @@ -638,6 +698,10 @@ Paul> } Paul> GDB_PY_HANDLE_EXCEPTION (except); Paul> +#ifdef IS_PY3K Paul> + PyBuffer_Release (&pybuf); Paul> +#endif It seems like PyBuffer_Release must be called before GDB_PY_HANDLE_EXCEPTION here. Paul> +#ifdef IS_PY3K Paul> +static PyBufferProcs buffer_procs = { Blank line between these two, and I think "{" on its own line is generally preferred. Paul> +++ gdb/python/py-value.c 14 Nov 2012 15:57:17 -0000 Paul> @@ -106,7 +106,7 @@ Paul> Py_XDECREF (self->dynamic_type); Paul> - self->ob_type->tp_free (self); Paul> + obj->ob_type->tp_free (self); Paul> } Doesn't this need the new Py_TYPE macro? Paul> +/* FIXME: there are a lot of calls below that do not check the return Paul> + value for errors. */ Don't bother with the new FIXME. Paul> + int python_init_ok = 0; This seems to be unused. Paul> + progname_copy = PyMem_Malloc ((progsize + 1) * sizeof (wchar_t)); [...] Paul> + Py_SetProgramName (progname_copy); I think a comment here explaining why we never free progname_copy would be nice to have. thanks, Tom