Re: [PATCH v7] Add an option with a color type.

[Andrei Pikas <gdb@mail.api.win>]

Hello.


> Andrei> +  if (*text == '\0')
> Andrei> +    {
> Andrei> +      /* Convenience to let the user know what the option
> Andrei> +	 can accept.  Note there's no common prefix between
> Andrei> +	 the strings on purpose, so that complete_on_enum doesn't do
> Andrei> +	 a partial match.  */
> Andrei> +      tracker.add_completion (make_unique_xstrdup ("NUMBER"));
> Andrei> +      tracker.add_completion (make_unique_xstrdup ("#RRGGBB"));
> Andrei> +    }
>
> This is a clever idea and I somewhat wish readline had a more robust
> completion API to show this kind of info "inline".
I didn't understand what I have to do about this.


> Andrei> +ui_file_style::color
> Andrei> +parse_cli_var_color (const char **args)
> Andrei> +{
> Andrei> +  /* Do a "set" command.  ARG is nullptr if no argument, or the
> Andrei> +     text of the argument.  */
> Andrei> +
> Andrei> +  if (args == nullptr || *args == nullptr || **args == '\0')
> Andrei> +    {
> Andrei> +      std::string msg;
> Andrei> +
> Andrei> +      for (size_t i = 0; ui_file_style::basic_color_enums[i]; ++i)
> Andrei> +	{
> Andrei> +	  msg.append ("\"");
> Andrei> +	  msg.append (ui_file_style::basic_color_enums[i]);
> Andrei> +	  msg.append ("\", ");
>
> I think this ends up appending an extra comma.  Perhaps the
> comma-addition should be done at the star of the loop, if "i > 0".
Yes, comma will be appended after the last color name.
But this is expected comma before the word integer.
The whole string will be
Requires an argument. Valid arguments are "none", "black", "red", 
"green", "yellow", "blue", "magenta", "cyan", "white", integer from -1 
to 255 or an RGB hex triplet in a format #RRGGBB
I will remove the extra space before integer in the next version of the 
patch.


> Andrei> diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
> Andrei> index 05539285c80..3a165ef6d2c 100644
> Andrei> --- a/gdb/cli/cli-option.c
> Andrei> +++ b/gdb/cli/cli-option.c
> Andrei> @@ -45,6 +45,9 @@ union option_value
>   
> Andrei>    /* For var_string options.  This is malloc-allocated.  */
> Andrei>    std::string *string;
> Andrei> +
> Andrei> +  /* For var_color options.  */
> Andrei> +  ui_file_style::color color = ui_file_style::NONE;
>
> I didn't even know inline initializers were possible in a union.
> Anyway I think it's probably better to leave this off.
There is a compilation error without this.


> Andrei> -  memset (&obj->value, 0, sizeof (obj->value));
> Andrei> +  obj->value = {}; /* zeros initialization */
>
> Was this needed?
Yes. parmpy_variable is not std::is_trivial now because of the 
ui_file_style::color. So memset from gdbsupport/poison.h fails on this type.
I will add explicit call to ~color() into parmpy_dealloc in the next 
version of the patch. Though this destructor is trivial at the moment.


> Andrei> +/* See ui-style.h.  */
> Andrei> +/* Must correspond to ui_file_style::basic_color.  */
> Andrei> +const std::vector<const char *> ui_file_style::basic_color_enums = {
> Andrei> +  "none",
> Andrei> +  "black",
> Andrei> +  "red",
> Andrei> +  "green",
> Andrei> +  "yellow",
> Andrei> +  "blue",
> Andrei> +  "magenta",
> Andrei> +  "cyan",
> Andrei> +  "white",
> Andrei> +  nullptr
> Andrei> +};
>
> I guess the nullptr is needed since although it is a vector now, it's
> still passed to some null-expecting API?  Like maybe completion?
>
> I wonder if a vector can be constexpr.
Yes, it's passed to complete_on_enum. Vector can't be constexpr.


> Andrei>  bool
> Andrei>  ui_file_style::color::append_ansi (bool is_fg, std::string *str) const
> Andrei>  {
> ...
> Andrei> +  else
> Andrei> +    return false;
>
> I wonder when this can happen and if it could somehow be made
> impossible.
No, this should no happen after all the checks in the constructors.
I will add gdb_assert_not_reached here and change return type to void in 
the next version of the patch.


On 04/10/2024 20:55, Tom Tromey wrote:
>>>>>> "Andrei" == Andrei Pikas<gdb@mail.api.win>  writes:
> Andrei> Colors can be specified as "none" for terminal's default color, as a name of
> Andrei> one of the eight standard colors of ISO/IEC 6429 "black", "red", "green", etc.,
> Andrei> as an RGB hexadecimal tripplet #RRGGBB for 24-bit TrueColor, or as an
> Andrei> integer from 0 to 255.  Integers 0 to 7 are the synonyms for the standard
> Andrei> colors.  Integers 8-15 are used for the so-called bright colors from the
> Andrei> aixterm extended 16-color palette.  Integers 16-255 are the indexes into xterm
> Andrei> extended 256-color palette (usually 6x6x6 cube plus gray ramp).  In
> Andrei> general, 256-color palette is terminal dependent and sometimes can be
> Andrei> changed with OSC 4 sequences, e.g. "\033]4;1;rgb:00/FF/00\033\\".
>
> Thank you for the patch.  I'm sorry about the long delays on this; gdb
> has an ongoing review bandwidth problem.  Anyway, I like this a lot and
> I would like to see it get in.
>
> I have a fair number of notes here but nothing really major.
>
> Andrei> +    case var_color:
> Andrei> +      {
> Andrei> +	std::string s = var.get<ui_file_style::color> ().to_string ();
> Andrei> +	return value_cstring (s.c_str (), s.size (),
> Andrei> +			      builtin_type (gdbarch)->builtin_char);
>
> Other code in this function uses:
>
> 	return current_language->value_string (gdbarch, value, len);
>
> Andrei> +  if (*text == '\0')
> Andrei> +    {
> Andrei> +      /* Convenience to let the user know what the option
> Andrei> +	 can accept.  Note there's no common prefix between
> Andrei> +	 the strings on purpose, so that complete_on_enum doesn't do
> Andrei> +	 a partial match.  */
> Andrei> +      tracker.add_completion (make_unique_xstrdup ("NUMBER"));
> Andrei> +      tracker.add_completion (make_unique_xstrdup ("#RRGGBB"));
> Andrei> +    }
>
> This is a clever idea and I somewhat wish readline had a more robust
> completion API to show this kind of info "inline".
>
> Andrei> +ui_file_style::color
> Andrei> +parse_cli_var_color (const char **args)
> Andrei> +{
> Andrei> +  /* Do a "set" command.  ARG is nullptr if no argument, or the
> Andrei> +     text of the argument.  */
> Andrei> +
> Andrei> +  if (args == nullptr || *args == nullptr || **args == '\0')
> Andrei> +    {
> Andrei> +      std::string msg;
> Andrei> +
> Andrei> +      for (size_t i = 0; ui_file_style::basic_color_enums[i]; ++i)
> Andrei> +	{
> Andrei> +	  msg.append ("\"");
> Andrei> +	  msg.append (ui_file_style::basic_color_enums[i]);
> Andrei> +	  msg.append ("\", ");
>
> I think this ends up appending an extra comma.  Perhaps the
> comma-addition should be done at the star of the loop, if "i > 0".
>
> Andrei> diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
> Andrei> index 05539285c80..3a165ef6d2c 100644
> Andrei> --- a/gdb/cli/cli-option.c
> Andrei> +++ b/gdb/cli/cli-option.c
> Andrei> @@ -45,6 +45,9 @@ union option_value
>   
> Andrei>    /* For var_string options.  This is malloc-allocated.  */
> Andrei>    std::string *string;
> Andrei> +
> Andrei> +  /* For var_color options.  */
> Andrei> +  ui_file_style::color color = ui_file_style::NONE;
>
> I didn't even know inline initializers were possible in a union.
> Anyway I think it's probably better to leave this off.
>
> Andrei> +@node Colors In Python
> Andrei> +@subsection Python representation of colors
>
> This new node doesn't seem to be referenced from a menu anywhere.  I
> think it has to be added to the appropriate spot in the menu in the
> "Python API" node.
>
> Andrei> diff --git a/gdb/python/py-color.c b/gdb/python/py-color.c
> Andrei> new file mode 100644
> Andrei> index 00000000000..1d72cc3e7d1
> Andrei> --- /dev/null
> Andrei> +++ b/gdb/python/py-color.c
> Andrei> @@ -0,0 +1,347 @@
> Andrei> +/* Python interface to ui_file_style::color objects.
> Andrei> +
> Andrei> +   Copyright (C) 2008-2022 Free Software Foundation, Inc.
>
> I suspect this should just be 2024.
>
> Andrei> +/* See py-color.h.  */
> Andrei> +bool
> Andrei> +gdbpy_is_color (PyObject *obj)
> Andrei> +{
> Andrei> +  return PyObject_IsInstance (obj, (PyObject *)&colorpy_object_type);
>
> gdb style is a space after the closing paren of a cast.
>
> Andrei> +static PyObject *
> Andrei> +colorpy_escape_sequence (PyObject *self, PyObject *is_fg_obj)
> Andrei> +{
> ...
> Andrei> +  bool is_fg = PyObject_IsTrue (is_fg_obj);
>
> In theory, this can fail, returning -1.
>
> In practice, the code already calls PyBool_Check, so I think just a
> direct comparison against Py_True would be fine.
>
> Andrei> +static int
> Andrei> +colorpy_init (PyObject *self, PyObject *args, PyObject *kwds)
> ...
> Andrei> +      if (colorspace_obj)
> Andrei> +	{
> Andrei> +	  if (PyLong_Check (colorspace_obj))
> Andrei> +	    {
> Andrei> +	      long colorspace_id = -1;
> Andrei> +	      if (! gdb_py_int_as_long (colorspace_obj, &colorspace_id))
> Andrei> +		return -1;
> Andrei> +	      if (colorspace_id < INT_MIN || colorspace_id > INT_MAX)
> Andrei> +		error (_("colorspace %ld is out of range."), colorspace_id);
>
> It seems like the range check here should use the colorspace enum values.
>
> Andrei> +	  Py_ssize_t tuple_size = PyTuple_Size (value_obj);
> Andrei> +	  if (tuple_size != 3)
> Andrei> +	    error (_("Tuple value with RGB must be of size 3."));
>
> If PyTuple_Size returns -1, this should early-return.
>
> Andrei> +	  if (!str)
> Andrei> +	    return -1;
>
> gdb uses comparisons like 'str == nullptr' here.
>
> Andrei> +	  if (colorspace_obj != nullptr
> Andrei> +	      && colorspace != obj->color.colorspace ())
> Andrei> +	    error (_("colorspace doesn't match to value."));
>
> I would s/to/the in this error message.
>
> Andrei> +  catch (const gdb_exception &except)
> Andrei> +    {
> Andrei> +      gdbpy_convert_exception (except);
> Andrei> +      return -1;
> Andrei> +    }
>
> A newish change landed and now this is written
>
>      return gdbpy_handle_exception (-1, except);
>
> Andrei> +/* Deallocate function for a gdb.Color.  */
> Andrei> +
> Andrei> +static void
> Andrei> +colorpy_dealloc (PyObject *)
> Andrei> +{
> Andrei> +}
>
> I wonder if this is necessary.
> If so maybe a comment explaining why would be good.
>
> Andrei> +/* Initialize the 'color' module.  */
> Andrei> +static int
> Andrei> +gdbpy_initialize_color (void)
> Andrei> +{
> Andrei> +  colorpy_object_type.tp_new = PyType_GenericNew;
> Andrei> +  if (PyType_Ready (&colorpy_object_type) < 0)
> Andrei> +    return -1;
>
> A recent change means that this should now call gdbpy_type_ready
> instead.  This also removes the need to call gdb_pymodule_addobject, so
> this call can be lowered to the end of this function.
>
> Andrei> diff --git a/gdb/python/py-color.h b/gdb/python/py-color.h
> ...
> Andrei> +/* Extracts value from OBJ object of gdb.Color type.  */
> Andrei> +extern const ui_file_style::color & gdbpy_get_color (PyObject *obj);
>
> No space after the "&".
>
> Andrei> -  memset (&obj->value, 0, sizeof (obj->value));
> Andrei> +  obj->value = {}; /* zeros initialization */
>
> Was this needed?
>
> Andrei> diff --git a/gdb/testsuite/gdb.guile/scm-color.exp b/gdb/testsuite/gdb.guile/scm-color.exp
> Andrei> new file mode 100644
> Andrei> index 00000000000..e5773650f2f
> Andrei> --- /dev/null
> Andrei> +++ b/gdb/testsuite/gdb.guile/scm-color.exp
> Andrei> @@ -0,0 +1,110 @@
> Andrei> +# Copyright (C) 2010-2022 Free Software Foundation, Inc.
>
> Another date to change.  I'd suggest looking at all of them.
>
> Andrei> diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp
> Andrei> index de524f49ad6..6e6a6007198 100644
> Andrei> --- a/gdb/testsuite/gdb.python/py-parameter.exp
> Andrei> +++ b/gdb/testsuite/gdb.python/py-parameter.exp
> Andrei> @@ -185,6 +185,58 @@ proc_with_prefix test_enum_parameter { } {
> Andrei>  	"Undefined item: \"three\".*" "set invalid enum parameter"
> Andrei>  }
>   
> Andrei> +# Test an color parameter.
> Andrei> +proc_with_prefix test_color_parameter { } {
> Andrei> +    global env
> Andrei> +    save_vars { env(TERM) env(COLORTERM) } {
>
> This should probably use with_ansi_styling_terminal to avoid having
> NO_COLOR set?
>
> Andrei> +static void
> Andrei> +init_colorsupport_var (void)
>
> You should use "()" instead of "(void)".  The latter is a leftover
> C-ism.
>
> Andrei> +{
> Andrei> +  const std::vector<color_space>& cs = colorsupport ();
>
> Also move the "&" after the space.
>
> Andrei> +/* See ui-style.h.  */
> Andrei> +/* Must correspond to ui_file_style::basic_color.  */
> Andrei> +const std::vector<const char *> ui_file_style::basic_color_enums = {
> Andrei> +  "none",
> Andrei> +  "black",
> Andrei> +  "red",
> Andrei> +  "green",
> Andrei> +  "yellow",
> Andrei> +  "blue",
> Andrei> +  "magenta",
> Andrei> +  "cyan",
> Andrei> +  "white",
> Andrei> +  nullptr
> Andrei> +};
>
> I guess the nullptr is needed since although it is a vector now, it's
> still passed to some null-expecting API?  Like maybe completion?
>
> I wonder if a vector can be constexpr.
>
> Andrei>  bool
> Andrei>  ui_file_style::color::append_ansi (bool is_fg, std::string *str) const
> Andrei>  {
> ...
> Andrei> +  else
> Andrei> +    return false;
>
> I wonder when this can happen and if it could somehow be made
> impossible.
>
> Andrei> +const std::vector<color_space> &
> Andrei> +colorsupport ()
> Andrei> +{
> Andrei> +  static const std::vector<color_space> value = []
>
> I didn't realize you could omit the '()' from a lambda.
>
> Andrei> +    {
> Andrei> +      std::vector<color_space> result = {color_space::MONOCHROME};
> Andrei> +
> Andrei> +      int colors = tgetnum ((char *)"Co");
>
> Is the cast really needed?
>
> Andrei> +      const char *colorterm = getenv ("COLORTERM");
> Andrei> +      if (colorterm && (!strcmp (colorterm, "truecolor")
>
> 'colorterm != nullptr'
>
> Andrei> +const char *
> Andrei> +color_space_name (color_space c)
> Andrei> +{
> Andrei> +  switch (c)
> Andrei> +    {
> Andrei> +    case color_space::MONOCHROME: return "monochrome";
> Andrei> +    case color_space::ANSI_8COLOR: return "ansi_8color";
> Andrei> +    case color_space::AIXTERM_16COLOR: return "aixterm_16color";
> Andrei> +    case color_space::XTERM_256COLOR: return "xterm_256color";
> Andrei> +    case color_space::RGB_24BIT: return "rgb_24bit";
> Andrei> +    }
> Andrei> +  error (_("Color space %d has no name."), static_cast<int> (c));
>
> Probably an assert-not-reached is more appropriate here?
>
> Andrei> +}
> Andrei> diff --git a/gdb/ui-style.h b/gdb/ui-style.h
> Andrei> index 1b7b5fafb9d..a13618dfce5 100644
> Andrei> --- a/gdb/ui-style.h
> Andrei> +++ b/gdb/ui-style.h
> Andrei> @@ -19,6 +19,26 @@
> Andrei>  #ifndef UI_STYLE_H
> Andrei>  #define UI_STYLE_H
>   
> Andrei> +/* One of the color spaces that usually supported by terminals.  */
> Andrei> +enum class color_space
> Andrei> +{
> Andrei> +  MONOCHROME, // one default terminal color
> Andrei> +  ANSI_8COLOR, // foreground colors \e[30m ... \e[37m,
> Andrei> +	       // background colors \e[40m ... \e[47m
> Andrei> +  AIXTERM_16COLOR, // foreground colors \e[30m ... \e[37m, \e[90m ... \e[97m
> Andrei> +		   // background colors \e[40m ... \e[47m, \e[100m ... \e107m
> Andrei> +  XTERM_256COLOR, // foreground colors \e[38;5;0m ... \e[38;5;255m
> Andrei> +		  // background colors \e[48;5;0m ... \e[48;5;255m
> Andrei> +  RGB_24BIT // foreground colors \e[38;2;0;0;0m ... \e[38;2;255;255;255m
> Andrei> +	    // background colors \e[48;2;0;0;0m ... \e[48;2;255;255;255m
>
> gdb doesn't use "//" comments.  Also normally gdb doesn't use trailing
> comments on the line; instead I'd put them before each constant they
> describe.
>
> Tom