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

[Tom Tromey <tom@tromey.com>]

>>>>> "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