From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21859 invoked by alias); 28 Feb 2011 18:41:10 -0000 Received: (qmail 21808 invoked by uid 22791); 28 Feb 2011 18:41:07 -0000 X-SWARE-Spam-Status: No, hits=-6.9 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, 28 Feb 2011 18:40:58 +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 p1SIevQR015525 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 28 Feb 2011 13:40:57 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p1SIeuGL021447; Mon, 28 Feb 2011 13:40:56 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p1SIetLk016733; Mon, 28 Feb 2011 13:40:56 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id A61533784EE; Mon, 28 Feb 2011 11:40:55 -0700 (MST) From: Tom Tromey To: pmuldoon@redhat.com Cc: gdb-patches@sourceware.org Subject: Re: [patch] Implement set/show callback functions in gdb.Parameter References: Date: Mon, 28 Feb 2011 18:41:00 -0000 In-Reply-To: (Phil Muldoon's message of "Tue, 15 Feb 2011 16:24:47 +0000") 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 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-02/txt/msg00930.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> + PyObject *ds_obj = PyObject_GetAttr (object, attr); Phil> + Phil> + if (ds_obj && gdbpy_is_string (ds_obj)) Phil> + { Phil> + result = python_string_to_host_string (ds_obj); Phil> + if (result == NULL) Phil> + gdbpy_print_stack (); You need to decref ds_obj. Phil> +static char * Phil> +call_doc_function (PyObject *obj, PyObject *method, PyObject *arg) Phil> +{ Phil> + char *data = NULL; Phil> + struct cleanup *cleanup = make_cleanup (null_cleanup, NULL); Phil> + PyObject *result = PyObject_CallMethodObjArgs (obj, method, arg, NULL); Phil> + Phil> + if (! result) Phil> + return NULL; This early return would leak the null cleanup. It is somewhat weird to use cleanups in "pure Python-facing" code. Here it is more normal to just use ordinary code, calling decref on the right paths. Phil> +static void Phil> +get_set_value (char *args, int from_tty, Phil> + struct cmd_list_element *c) Phil> +{ Phil> + PyObject *obj = (PyObject *) get_cmd_context (c); Phil> + char *set_doc_string; Phil> + PyObject *set_doc_func = PyString_FromString ("get_set_string"); Phil> + struct cleanup *cleanup = ensure_python_env (get_current_arch (), Phil> + current_language); You can't call any Python API, in this case PyString_FromString, before calling ensure_python_env. Phil> + make_cleanup (xfree, set_doc_string); Phil> + fprintf_filtered (gdb_stdout,"%s\n", set_doc_string); Missing space after the first ",". Phil> +static void Phil> +get_show_value (struct ui_file *file, int from_tty, Phil> + struct cmd_list_element *c, Phil> + const char *value) Phil> +{ Phil> + PyObject *obj = (PyObject *) get_cmd_context (c); Phil> + char *show_doc_string = NULL; Phil> + PyObject *show_doc_func = PyString_FromString ("get_show_string"); Phil> + struct cleanup *cleanup = ensure_python_env (get_current_arch (), Phil> + current_language); Likewise about the ordering. Phil> + PyObject *result; I think this is unused. Phil> + fprintf_filtered (file,"%s\n", show_doc_string); Missing space. Phil> + make_cleanup(xfree, show_doc_string); Phil> + fprintf_filtered (file,"%s %s\n", show_doc_string, value); Missing a space on each line. Phil> + switch (parmclass) { case var_boolean: These lines got joined by mistake. Phil> + /* Lookup created parameter, and register Python object against the Phil> + parameter context. Perform this task against both lists. */ Phil> + tmp_name = cmd_name; Phil> + param = lookup_cmd (&tmp_name, *show_list, "", 0, 1); Phil> + if (param) Phil> + set_cmd_context (param, self); Phil> + Phil> + tmp_name = cmd_name; Phil> + param = lookup_cmd (&tmp_name, *set_list, "", 0, 1); Phil> + if (param) Phil> + set_cmd_context (param, self); This is pretty gross. I don't really understand the logic behind why some add_* functions return the command object and others do not. I guess in this case you'd have to have the code return two command objects, making the resulting function signature really ridiculous. No change needed here, IMO, I just wanted to complain. Tom