Re: [PATCH v2 3/5] Create MI commands using python.

Jan Vrany
2019-05-28T20:35:44Z

Hi,

thanks a lot! I agree with most of comments and fixed
them already in upcoming v3 patch.

For the rest, sae below.

> > > + {
> > > + /* Since PyList_SetItem steals the reference, we don't use
> > > + * gdbpy_ref<> to hold on arg string. */
> > > + PyObject *str = PyUnicode_Decode (parse->argv[i], strlen (parse->argv[i]),
> > > + host_charset (), NULL);
> > > + if (PyList_SetItem (argobj.get (), i, str) != 0)
> >
> > Just wondering, because I know that unicode handling is very different between
> > Python 2 and 3: did you test with both Python versions?
> >

No. I will do that before sending v3.

> > > + gdbpy_ref<> result (
> > > + PyObject_CallMethodObjArgs (obj, invoke_cst, argobj.get (), NULL));
> > > + if (PyErr_Occurred () != NULL)
> > > + {
> > > + gdbpy_err_fetch ex;
> > > + gdb::unique_xmalloc_ptr<char> ex_msg (ex.to_string ());
> > > +
> > > + if (ex_msg == NULL || *ex_msg == '\0')
> > > + error (_("-%s: failed to execute command"), name ().c_str ());
> > > + else
> > > + error (_("-%s: %s"), name ().c_str (), ex_msg.get ());
> > > + }
> > > + else if (result != nullptr)
> > > + {
> > > + if (Py_None != result)
> > > + parse_mi_result (result.get (), "result");
> > > + }
> > > + else
> > > + {
> > > + error (
> > > + _("-%s: command invoke() method returned NULL but no python exception "
> > > + "is set"),
> > > + name ().c_str ());
> >
> > I am curious, did you actually hit this case, where no Python exception was
> > raised and invoke returned NULL? I thought the two were pretty coupled
> > (if return value is NULL, it means there's an exception raise and vice versa).
> >

Yes, it looks like. I guess I took this code from Didier. I removed the else
branch in v3.

> > > + void invoke (struct mi_parse *parse) override;
> >
> > This should be private, like in the previous patch.
> >

Ah, this is "bigger" problem, we should override
do_invoke() which is declared protected in superclass. Again,fixed in v3.

Thanks a lot!

Jan