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

Tom Tromey
2025-09-16T16:48:34Z

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

Andrew> + gdb::unique_xmalloc_ptr<char>
Andrew> + input (python_string_to_target_string (input_obj));

target string seems like an unusual choice here.

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.

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.

Tom