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

Andrew Burgess
2025-09-23T16:59:44Z

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