[RFC 3/8] Create MI commands using python.
Tom Tromey
tom@tromey.com
Thu Apr 25 19:42:00 GMT 2019
>>>>> "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
More information about the Gdb-patches
mailing list