Re: [PATCHv8 3/3] gdb/python: extend gdb.write to support styled output

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> Using gdb.write with a style object more closely matches how GDB
> Andrew> handles styling internally, and has the benefit that the user doesn't
> Andrew> need to remember to restore the default style when they are done.
>
> I'm mildly meh on this one since I tend to think users should use print
> and that gdb.write is an implementation detail.  But since it exists...
>
> Andrew> +  return PyObject_TypeCheck (obj, &style_object_type);
>
> Andrew> +/* See python-internal.h.   */
> Andrew> +
> Andrew> +std::optional<ui_file_style>
> Andrew> +gdbpy_style_object_to_ui_file_style (PyObject *obj)
> Andrew> +{
> Andrew> +  gdb_assert (obj != nullptr);
> Andrew> +  if (!gdbpy_is_style (obj))
> Andrew> +    {
> Andrew> +      PyErr_Format
> Andrew> +	(PyExc_TypeError, _("argument must be a gdb.Style object, not %s."),
> Andrew> +	 Py_TYPE (obj)->tp_name);
> Andrew> +      return {};
> Andrew> +    }
> Andrew> +
> Andrew> +  style_object *style_obj = (style_object *) obj;
> Andrew> +  return stylepy_to_style (style_obj);
>
>
> Andrew> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|iO", keywords, &arg,
> Andrew> +					&stream_type, &style_obj))
> Andrew> +    return nullptr;
> Andrew> +
> Andrew> +  if (style_obj != Py_None && !gdbpy_is_style (style_obj))
> Andrew> +    {
> Andrew> +      PyErr_Format
> Andrew> +	(PyExc_TypeError,
> Andrew> +	 _("'style' argument must be gdb.Style or None, not %s."),
> Andrew> +	 Py_TYPE (style_obj)->tp_name);
> Andrew> +      return nullptr;
>
> FWIW if the type object is available there's a way to get Python to do
> this bit when parsing the arguments.

I think you're referring too 'O!' maybe?  The reason that doesn't work
is that the argument can be a gdb.Style _or_ None.

It feels like this must be a common pattern, so I would have expected to
find a modifier that takes a named type or accepts None, but I couldn't
find anything.

Is there another trick that I'm not aware of?  Would love to know
because I'm pretty sure I've had to do this elsewhere in GDB when an
argument can also be None.

> This explicit check is fine too, but it seems to me that this is
> redundant with the check that gdbpy_style_object_to_ui_file_style does
> and so this could just delegate to that.

Given gdbpy_style_object_to_ui_file_style is only used in this one
place, what I did in the end was to drop the gdbpy_is_style check from
gdbpy_style_object_to_ui_file_style, replacing it with an assert.

Then I've retained the check in gdbpy_write.

The reason is that the error message in gdbpy_write is so much more
descriptive, it names the argument, and says that the argument can be a
gdb.Style or None.  The error from gdbpy_style_object_to_ui_file_style
was much more generic.  I prefer accurate error messages wherever
possible.

So, after this change, there's only one gdbpy_is_style check (not
counting the assert), and only one place where the error message is set
if the argument is the wrong type.

I posted a v9 series with these changes in, let me know what you think.

Thanks,
Andrew