Re: [PATCHv8 1/3] gdb/python: add gdb.Style class

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> +static std::optional<ui_file_style>
> Andrew> +stylepy_style_from_name (const char *name, bool *has_intensity_ptr = nullptr,
> Andrew> +			 const cmd_list_element **found_cmd_ptr = nullptr)
> Andrew> +{
> Andrew> +  std::string cmd_str = std::string ("show style ") + name;
> Andrew> +
> Andrew> +  struct cmd_list_element *cmd = nullptr;
> Andrew> +  struct cmd_list_element *alias = nullptr;
> Andrew> +  int found;
> Andrew> +  try
> Andrew> +    {
> Andrew> +      struct cmd_list_element *prefix;
> Andrew> +      found = lookup_cmd_composition (cmd_str.c_str (), &alias, &prefix, &cmd);
> Andrew> +    }
> Andrew> +  catch (const gdb_exception &ex)
> Andrew> +    {
> Andrew> +      PyErr_Format (PyExc_RuntimeError,
> Andrew> +		    _("style '%s' cannot be found."), name);
> Andrew> +      return {};
>
> Noting this for a later comment...
>
> Andrew> +static PyObject *
> Andrew> +stylepy_apply (PyObject *self, PyObject *args, PyObject *kw)
> Andrew> +{
> Andrew> +  style_object *style_obj = (style_object *) self;
> Andrew> +
> Andrew> +  static const char *keywords[] = { "string", nullptr };
> Andrew> +  PyObject *input_obj;
> Andrew> +
> Andrew> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "O!", keywords,
> Andrew> +					&PyUnicode_Type, &input_obj))
> Andrew> +    return nullptr;
>
> Why "O!" and not "s" here?

This is possibly a little premature optimisation, but capturing the
string as O! means I get a 'PyObject *'.  In the case where styling is
not applied I can then just Py_INCREF that object and return it.

If I capture the argument with "s" then I get a 'char *' back, so in the
case where no styling is being added I have to create a new PyObject by
converting the string _back_ again.

I've left this code as it is for now, but I have added a comment.  If
you dislike this then just let me know, it's not essential, and I can
switch to using "s".

>
> Andrew> +      gdb::unique_xmalloc_ptr<char>
> Andrew> +	input (python_string_to_target_string (input_obj));
>
> target string seems like an unusual choice here.

Yeah, especially as I use host_string_to_python_string about 5 lines
later to convert the adjusted string back to a Python object.  I've
fixed this to use python_string_to_host_string.

>
> Andrew> +      std::string output
> Andrew> +	= string_printf ("%s%s%s", style->to_ansi ().c_str (), input.get (),
> Andrew> +			 ui_file_style ().to_ansi ().c_str ());
>
> This can just using "+" which seems a little cleaner.

Done.

>
> Andrew> +  /* Handle named styles.  This is harder, we need to write to the actual
> Andrew> +     parameter.  First though, look up the named style to see if it has an
> Andrew> +     intensity.  HAS_INTENSITY will be set true only if the style exists,
> Andrew> +     and has an 'intensity' setting.  */
> Andrew> +
> Andrew> +  bool has_intensity = false;
> Andrew> +  (void) stylepy_style_from_name (style_obj->style_name, &has_intensity);
> Andrew> +  if (!has_intensity)
> Andrew> +    {
> Andrew> +      PyErr_Format
> Andrew> +	(PyExc_ValueError, "the intensity of style '%s' is not writable.",
> Andrew> +	 style_obj->style_name);
> Andrew> +      return -1;
>
> I guess I don't know the Python rule here, but it seems like if
> stylepy_style_from_name returns {}, then this can set an exception while
> one is already set.  See the snippet above.
>
> Instead of the (void) I think it would be a bit better to just check the
> return result of stylepy_style_from_name, and if it returns an error,
> just return -1 directly; and only then do the has_intensity check.

You are absolutely correct.  I fixed this, capturing the return value
and checking we found a style.

I've posted a v9 series with all these fixes.

Thanks,
Andrew