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

[Tom Tromey <tom@tromey.com>]

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