Re: [patch] Implement set/show callback functions in gdb.Parameter

[Tom Tromey <tromey@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