This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Implement set/show callback functions in gdb.Parameter
- From: Tom Tromey <tromey at redhat dot com>
- To: pmuldoon at redhat dot com
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 28 Feb 2011 11:40:55 -0700
- Subject: Re: [patch] Implement set/show callback functions in gdb.Parameter
- References: <m3bp2dw9io.fsf@redhat.com>
>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> 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