Re: [RFC 3/8] Create MI commands using python.

Tom Tromey
2019-04-25T19:42:38Z

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Lots of nits on this one, mostly I think because it was converted from
C? Anything, nothing very major, just lots of little things.

Jan> +mi_command_py::mi_command_py (const char *name, int *suppress_notification,
Jan> + void *object)
Jan> + : mi_command (name, suppress_notification),
Jan> + pyobj (object)
Jan> +{}

I think it would be better to put this sort of thing somewhere in the
Python code, so that it can use gdbpy_ref<> rather than "void *".

Jan> + py_mi_invoke (this->pyobj, parse->argv, parse->argc);

This sort of indirection wouldn't be needed then either.

Jan> +class mi_command_py : public mi_command
Jan> +{
Jan> + public:
Jan> + mi_command_py (const char *name, int *suppress_notification,
Jan> + void *object);
Jan> + void invoke (struct mi_parse *parse) override;
Jan> +
Jan> + private:
Jan> + void *pyobj;
Jan> +
Jan> +};

Extra blank line before the "}".

Jan> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
Jan> new file mode 100644
Jan> index 0000000000..ee612e2bc5
Jan> --- /dev/null
Jan> +++ b/gdb/python/py-micmd.c
Jan> @@ -0,0 +1,290 @@
Jan> +/* gdb MI commands implemented in Python */
Jan> +
Jan> +#include <string.h>

Files need a copyright header.

Jan> +#include "defs.h"

defs.h must come first. I wonder if string.h is needed at all.

Jan> +/* If the command invoked returns a list, this function parses it and create an
Jan> + appropriate MI out output.
Jan> +
Jan> + The returned values must be Python string, and can be contained within Python
Jan> + lists and dictionaries. It is possible to have a multiple levels of lists
Jan> + and/or dictionaries. */
Jan> +
Jan> +static void
Jan> +parse_mi_result (PyObject *result, const char *field_name)
Jan> +{
Jan> + struct ui_out *uiout = current_uiout;
Jan> +
Jan> + if(!field_name)

gdb+gnu style would be: "if (field_name == nullptr)"

Jan> + if (PyString_Check(result))

Need a space before open parens.
There are a few instances of this.

Jan> + {
Jan> + const char *string = gdbpy_obj_to_string (result).release ();
Jan> + uiout->field_string (field_name, string);
Jan> + xfree ( (void *)string);

Better to take advantage of the unique pointer, like:

gdb::unique_xmalloc_ptr<char> string = gdbpy_obj_to_string (result);
uiout->field_string (field_name, string.get ());

However, this code is ignoring Python errors. Probably this function
should use one of the standard conventions and return -1 on error.

Jan> + //struct cleanup *cleanup_item = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);

There are a couple of these; remove them.

Jan> + item = PyList_GetItem (result, i);

This can fail, so it needs a check.

Jan> + else if ( PyDict_Check (result))

There's an extra space after paren.

Jan> + char *key_string = gdbpy_obj_to_string (key).release ();

Error checking.

Jan> + xfree ( (void *) key_string);

Again, no need to do this explicitly.

Jan> + PyObject *argobj, *result, **strings;

Extra space.

Jan> + if (! obj)
Jan> + error(_("Invalid invocation of Python micommand object."));

This could probably be improved somehow, for instance mentioning the MI
command name. I think that's sort of standard in MI commands, so this
should follow it in all the errors, I think.

Jan> + strings = (PyObject **) malloc (sizeof(PyObject *) * argc);

Not sure this is needed, see below. But if it is it should use XNEWVEC
or xmalloc.

Jan> + for (i = 0; i < argc; ++i)
Jan> + {
Jan> + strings[i] = PyUnicode_Decode (argv[i], strlen(argv[i]), host_charset (), NULL);
Jan> + if (PyList_SetItem (argobj, i, strings[i]) != 0)

PyList_SetItem steals a reference, so I think the ref-counting in this
function is incorrect. But, since it steals a reference, I think
"strings" isn't needed.

Jan> + {
Jan> + error (_("Failed to create the python arguments list."));

This will leak references. However if you use gdbpy_ref<>, you should
be ok.

Jan> + /* Skip preceding whitespaces. */
Jan> + /* Find first character of the final word. */

Not sure if the first comment documents something that should be there
and is not, or if it should just be removed.

Probably the latter, I think it's fine to require command names to be
valid. I'd probably skip stripping the trailing whitespace as well.

Jan> + cmd_name = micmdpy_parse_command_name (name);
Jan> + if (! cmd_name)
Jan> + return -1;

Like this could just give an error if the NAME isn't a valid MI command
name, and not bother with trying to fix it up, or copy it.

Jan> + PyErr_Format (except.reason == RETURN_QUIT
Jan> + ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
Jan> + "%s", except.message);

I think GDB_PY_SET_HANDLE_EXCEPTION is better for this.

Jan> +/* Initialize the MI command object. */
Jan> +
Jan> +int
Jan> +gdbpy_initialize_micommands(void)

Don't need (void) any more.

Jan> +++ b/gdb/python/py-micmd.h
Jan> @@ -0,0 +1,6 @@
Jan> +#ifndef PY_MICMDS_H
Jan> +#define PY_MICMDS_H
Jan> +
Jan> +void py_mi_invoke (void *py_obj, char **argv, int argc);
Jan> +

I'd just stick this in python-internal.h and not have a new header.

thanks,
Tom