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

[Jan Vrany <jan.vrany@fit.cvut.cz>]

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