From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 432963858D1E for ; Wed, 17 Aug 2022 17:41:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 432963858D1E Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-196-4hwJu_apNcSd9064nwBrBA-1; Wed, 17 Aug 2022 13:41:02 -0400 X-MC-Unique: 4hwJu_apNcSd9064nwBrBA-1 Received: by mail-wm1-f71.google.com with SMTP id a17-20020a05600c349100b003a545125f6eso1368488wmq.4 for ; Wed, 17 Aug 2022 10:41:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc; bh=wVVufjVzs0sz0XESMTyaGV/O5TCkfKqEO9waOO4aIMo=; b=0ikVsoHpt+TtsLHynN8XeS1QmvrD/+9X69cEsoemsAtkUgPzbDt9dBHvlrti/gdLV8 aKrj2SPano7zcDyaOUKq2+YiORB3lCq7rh7W1SdXULH6G++pyljSScqTbCcK1iI0D6+K 87faxP1qNe5v4gLn8cU80/rOpU18E+OuJ276dhOk7L/x+Xd9JRolisfBdZTYihuqCNOo K/DlSjtCmztHohKtaiKgJXIBk8/4nvjd4cyjzAfERKlibHcDhbjKYmT73s4IJQo59JRI uVBXNv1PnbhJOObC8j6KSeGAkZET9PsQsHA5u80dLIi1YEX9kdAx/9CLcM3Ik+1S/X9X HEBg== X-Gm-Message-State: ACgBeo0p6n/2Qw5m7p7CGNMQQ3kE2H51YrJB/WMFxTBQ3E/2Luna4IOP aVgOjM0mmLPx+tU8M/QMCROOHmpC7VkjValje4CaRrDx+FBSeCdzPrOaafiwbN5burTQjdBC9jh kTJ1dK9oc7HG1ZY15rJpQrg== X-Received: by 2002:a05:600c:348d:b0:3a6:b4e:ff6d with SMTP id a13-20020a05600c348d00b003a60b4eff6dmr2686162wmq.95.1660758061018; Wed, 17 Aug 2022 10:41:01 -0700 (PDT) X-Google-Smtp-Source: AA6agR6CP01zRdmcfcRqUbu9LXawZEPm5u8jicAVtQyGqX4vQHQJzRNHJj90YZqz/XejZWXMKZMfQw== X-Received: by 2002:a05:600c:348d:b0:3a6:b4e:ff6d with SMTP id a13-20020a05600c348d00b003a60b4eff6dmr2686149wmq.95.1660758060341; Wed, 17 Aug 2022 10:41:00 -0700 (PDT) Received: from localhost ([31.111.84.229]) by smtp.gmail.com with ESMTPSA id g6-20020a5d4886000000b0021e9fafa601sm13282270wrq.22.2022.08.17.10.40.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 17 Aug 2022 10:40:59 -0700 (PDT) From: Andrew Burgess To: Andrei Pikas , gdb-patches@sourceware.org Cc: Andrei Pikas Subject: Re: [PATCH] Add an option with a color type. In-Reply-To: <20220815211534.33740-1-gdb@mail.api.win> References: <20220815211534.33740-1-gdb@mail.api.win> Date: Wed, 17 Aug 2022 18:40:58 +0100 Message-ID: <87zgg2ydhx.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Aug 2022 17:41:12 -0000 Thanks for working on this. I only had time for a quick skim through, but I had a few thoughts. Andrei Pikas writes: > Colors can be specified as name of one of the basic colors "none", "black", > "white", etc., as a number up to 255, or as RGB hexadecimal tripplet #RRGGBB. > --- > gdb/NEWS | 3 + > gdb/cli/cli-cmds.c | 8 ++ > gdb/cli/cli-decode.c | 200 +++++++++++++++++++++++++++++++ > gdb/cli/cli-decode.h | 30 +++++ > gdb/cli/cli-option.c | 42 +++++++ > gdb/cli/cli-option.h | 21 ++++ > gdb/cli/cli-setshow.c | 53 ++++++++ > gdb/cli/cli-setshow.h | 6 + > gdb/cli/cli-style.c | 49 ++------ > gdb/cli/cli-style.h | 4 +- > gdb/command.h | 26 +++- > gdb/guile/scm-param.c | 69 ++++++++++- > gdb/python/py-param.c | 63 +++++++++- > gdb/python/python.c | 18 +++ > gdb/testsuite/gdb.base/style.exp | 15 +++ > gdb/ui-style.h | 16 ++- I notice there's no gdb/doc/* changes here, that'll definitely need filling in. > 16 files changed, 575 insertions(+), 48 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index d2efe2a0a58..5234d4f8501 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -55,6 +55,9 @@ > Python Pygments is still used. For supported targets, libopcodes > styling is used by default. > > + "set style" commands now supports numeric format for basic colors > + from -1 to 255 and #RRGGBB format for truecolor. > + You should also mention the new Python and Guile API features as separate entries in the 'Python API' list and (might need adding) the 'Guile API' list. I am also not convinced the support for -1 as an option is needed. If I understand correctly, if a user does: set style filename foreground -1 this is the same as: set style filename foreground none which is already supported. So, I think we should just support 0->255 for the 256-color mode, and point users towards 'none' if they don't want styling. > * New commands > > maintenance set ignore-prologue-end-flag on|off > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index 18fb6e6d869..5039c665b16 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -2275,6 +2275,13 @@ value_from_setting (const setting &var, struct gdbarch *gdbarch) > return value_cstring ("", 1, > builtin_type (gdbarch)->builtin_char); > } > + case var_color: > + { > + char s[64]; > + print_color_str (s, sizeof s, var.get ()); > + size_t len = strlen (s); I think we should follow the pattern we have for many other types, in adding a ui_file_style::color::to_string () function that returns a std::string. > + return value_cstring (s, len, builtin_type (gdbarch)->builtin_char); > + } > default: > gdb_assert_not_reached ("bad var_type"); > } > @@ -2324,6 +2331,7 @@ str_value_from_setting (const setting &var, struct gdbarch *gdbarch) > case var_auto_boolean: > case var_uinteger: > case var_zuinteger: > + case var_color: > { > std::string cmd_val = get_setshow_command_value_string (var); > > diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c > index fde554c7e6c..1e36ae84123 100644 > --- a/gdb/cli/cli-decode.c > +++ b/gdb/cli/cli-decode.c > @@ -670,6 +670,112 @@ add_setshow_enum_cmd (const char *name, command_class theclass, > return cmds; > } > > +/* See cli-decode.h. */ > +/* Must correspond to ui_file_style::basic_color. */ > +const std::vector basic_color_enums = { > + "none", > + "black", > + "red", > + "green", > + "yellow", > + "blue", > + "magenta", > + "cyan", > + "white", > + nullptr > +}; > + > +/* See cli-decode.h. */ > + > +const char * basic_color_name (int color) The GNU style places the function name at the start of a line, like: const char * basic_color_name (int color) { ... } > +{ > + int pos = color - ui_file_style::NONE; > + if (0 <= pos && pos < basic_color_enums.size ()) > + if (const char *s = basic_color_enums[pos]) > + return s; > + error (_("Basic color %d has no name."), pos); > +} > + > +/* See cli-decode.h. */ > + > +void > +complete_on_color (completion_tracker &tracker, > + const char *text, const char *word) > +{ > + complete_on_enum (tracker, basic_color_enums.data (), text, word); > + if (*text == '\0') > + { > + /* Convenience to let the user know what the option > + can accept. Note there's no common prefix between > + the strings on purpose, so that readline doesn't do > + a partial match. */ > + tracker.add_completion (make_unique_xstrdup ("NUMBER")); > + tracker.add_completion (make_unique_xstrdup ("#RRGGBB")); > + } > +} > + > +/* Completer used in color commands. */ > + > +static void > +color_completer (struct cmd_list_element *ignore, > + completion_tracker &tracker, > + const char *text, const char *word) > +{ > + complete_on_color (tracker, text, word); > +} > + > + > +/* Add element named NAME to command list LIST (the list for set or > + some sublist thereof). CLASS is as in add_cmd. VAR is address > + of the variable which will contain the color. */ > + > +set_show_commands > +add_setshow_color_cmd (const char *name, > + enum command_class theclass, > + ui_file_style::color *var, > + const char *set_doc, > + const char *show_doc, > + const char *help_doc, > + cmd_func_ftype *set_func, > + show_value_ftype *show_func, > + struct cmd_list_element **set_list, > + struct cmd_list_element **show_list) > +{ > + set_show_commands commands = add_setshow_cmd_full > + (name, theclass, var_color, var, > + set_doc, show_doc, help_doc, > + nullptr, nullptr, set_func, show_func, > + set_list, show_list); > + > + set_cmd_completer (commands.set, color_completer); > + > + return commands; > +} > + > +/* Same as above but using a getter and a setter function instead of a pointer > + to a global storage buffer. */ > + > +set_show_commands > +add_setshow_color_cmd (const char *name, command_class theclass, > + const char *set_doc, const char *show_doc, > + const char *help_doc, > + setting_func_types::set set_func, > + setting_func_types::get get_func, > + show_value_ftype *show_func, > + cmd_list_element **set_list, > + cmd_list_element **show_list) > +{ > + auto cmds = add_setshow_cmd_full > + (name, theclass, var_color, nullptr, > + set_doc, show_doc, help_doc, > + set_func, get_func, nullptr, show_func, > + set_list, show_list); > + > + set_cmd_completer (cmds.set, color_completer); > + > + return cmds; > +} > + > /* See cli-decode.h. */ > const char * const auto_boolean_enums[] = { "on", "off", "auto", NULL }; > > @@ -2524,3 +2630,97 @@ cli_user_command_p (struct cmd_list_element *cmd) > { > return cmd->theclass == class_user && cmd->func == do_simple_func; > } > + > +/* See cli-decode.h. */ > + > +ui_file_style::color > +parse_cli_var_color (const char **args) > +{ > + /* Do a "set" command. ARG is NULL if no argument, or the > + text of the argument. */ > + > + if (args == nullptr || *args == nullptr || **args == '\0') > + { > + std::string msg; > + > + for (size_t i = 0; basic_color_enums[i]; ++i) > + msg.append ("\"").append (basic_color_enums[i]).append ("\", "); > + > + error (_("Requires an argument. Valid arguments are %s integer from -1 " > + "to 255 or an RGB hex triplet in a format #RRGGBB"), > + msg.c_str ()); > + } > + > + const char *p = skip_to_space (*args); > + size_t len = p - *args; > + > + int nmatches = 0; > + ui_file_style::basic_color match = ui_file_style::NONE; > + for (size_t i = 0; basic_color_enums[i]; ++i) > + if (strncmp (*args, basic_color_enums[i], len) == 0) > + { > + if (basic_color_enums[i][len] == '\0') > + { > + match = static_cast (i - 1); > + nmatches = 1; > + break; /* Exact match. */ > + } > + else > + { > + match = static_cast (i - 1); > + nmatches++; > + } > + } > + > + if (nmatches == 1) > + { > + *args += len; > + return ui_file_style::color (match); > + } > + > + if (nmatches > 1) > + error (_("Ambiguous item \"%.*s\"."), (int) len, *args); > + > + if (**args != '#') > + { > + char *end = nullptr; > + long num = strtol (*args, &end, 0); > + if (end - *args != len) > + error (_("Expected integer at: %s"), *args); > + if (num < -1 || num > 255) > + error (_("integer %s out of range"), plongest (num)); > + > + *args = end; > + return ui_file_style::color (static_cast (num)); > + } > + > + /* Try to parse #RRGGBB string. */ > + if (len != 7) > + error_no_arg (_("invalid RGB hex triplet format")); > + > + uint8_t r, g, b; > + int scanned_chars = 0; > + int parsed_args = sscanf (*args, "#%2" SCNx8 "%2" SCNx8 "%2" SCNx8 "%n", > + &r, &g, &b, &scanned_chars); > + > + if (parsed_args != 3 || scanned_chars != 7) > + error_no_arg (_("invalid RGB hex triplet format")); > + > + *args += len; > + return ui_file_style::color (r, g, b); > +} > + > +/* See cli-decode.h. */ > + > +ui_file_style::color parse_var_color (const char *arg) Newline after ::color. > +{ > + const char *end_arg = arg; > + ui_file_style::color color = parse_cli_var_color (&end_arg); > + > + int len = end_arg - arg; > + const char *after = skip_spaces (end_arg); > + if (*after != '\0') > + error (_("Junk after item \"%.*s\": %s"), len, arg, after); > + > + return color; > +} > diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h > index 18db8822af3..1f2ef80c7e9 100644 > --- a/gdb/cli/cli-decode.h > +++ b/gdb/cli/cli-decode.h > @@ -306,6 +306,36 @@ extern const char * const boolean_enums[]; > /* The enums of auto-boolean commands. */ > extern const char * const auto_boolean_enums[]; > > +/* The enums of color commands with names of the basic colors that > + can be handled by ANSI terminals. > + Corresponds to ui_file_style::basic_color and starts with name for > + ui_file_style::NONE. */ > +extern const std::vector basic_color_enums; > + > +/* Returns text representation of COLOR. */ > +extern const char * basic_color_name (int color); > + > +/* Add the different possible completions of TEXT with color. > + > + WORD points in the same buffer as TEXT, and completions should be > + returned relative to this position. For example, suppose TEXT is "foo" > + and we want to complete to "foobar". If WORD is "oo", return > + "oobar"; if WORD is "baz/foo", return "baz/foobar". */ > + > +void > +complete_on_color (completion_tracker &tracker, > + const char *text, const char *word); > + > +/* Parse ARGS, an option to a var_color variable. > + Either returns the parsed value on success or throws an error. > + ARGS may be one of strings {none, black, red, green, yellow, blue, magenta, > + cyan, white}, or color number from -1 to 255, or RGB hex triplet #RRGGBB. > + */ > +ui_file_style::color parse_cli_var_color (const char **args); > + > +/* Same as above but additionally check that there is no junk in the end. */ > +ui_file_style::color parse_var_color (const char *arg); I think these should be marked extern. > + > /* Verify whether a given cmd_list_element is a user-defined command. > Return 1 if it is user-defined. Return 0 otherwise. */ > > diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c > index b1794ad4b17..bd6556312db 100644 > --- a/gdb/cli/cli-option.c > +++ b/gdb/cli/cli-option.c > @@ -46,6 +46,9 @@ union option_value > > /* For var_string options. This is malloc-allocated. */ > std::string *string; > + > + /* For var_color options. */ > + ui_file_style::color color = ui_file_style::NONE; > }; > > /* Holds an options definition and its value. */ > @@ -424,6 +427,33 @@ parse_option (gdb::array_view options_group, > val.enumeration = parse_cli_var_enum (args, match->enums); > return option_def_and_value {*match, match_ctx, val}; > } > + case var_color: > + { > + if (completion != nullptr) > + { > + const char *after_arg = skip_to_space (*args); > + if (*after_arg == '\0') > + { > + complete_on_color (completion->tracker, *args, *args); > + > + if (completion->tracker.have_completions ()) > + return {}; > + } > + } > + > + if (check_for_argument (args, "--")) > + { > + /* Treat e.g., "backtrace -entry-values --" as if there > + was no argument after "-entry-values". This makes > + parse_cli_var_color throw an error with a suggestion of > + what are the valid options. */ > + args = nullptr; > + } > + > + option_value val; > + val.color = parse_cli_var_color (args); > + return option_def_and_value {*match, match_ctx, val}; > + } > case var_string: > { > if (check_for_argument (args, "--")) > @@ -601,6 +631,10 @@ save_option_value_in_ctx (gdb::optional &ov) > *ov->option.var_address.enumeration (ov->option, ov->ctx) > = ov->value->enumeration; > break; > + case var_color: > + *ov->option.var_address.color (ov->option, ov->ctx) > + = ov->value->color; > + break; > case var_string: > *ov->option.var_address.string (ov->option, ov->ctx) > = std::move (*ov->value->string); > @@ -677,6 +711,14 @@ get_val_type_str (const option_def &opt, std::string &buffer) > } > return buffer.c_str (); > } > + case var_color: > + { > + buffer = ""; > + for (size_t i = 0; basic_color_enums[i]; ++i) > + buffer.append (basic_color_enums[i]).append ("|"); > + buffer += "NUMBER|#RRGGBB"; > + return buffer.c_str (); > + } > case var_string: > return "STRING"; > default: > diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h > index 26a8da3a5a4..d30c7024324 100644 > --- a/gdb/cli/cli-option.h > +++ b/gdb/cli/cli-option.h > @@ -87,6 +87,7 @@ struct option_def > int *(*integer) (const option_def &, void *ctx); > const char **(*enumeration) (const option_def &, void *ctx); > std::string *(*string) (const option_def &, void *ctx); > + ui_file_style::color *(*color) (const option_def &, void *ctx); > } > var_address; > > @@ -282,6 +283,26 @@ struct string_option_def : option_def > } > }; > > +/* A var_color command line option. */ > + > +template > +struct color_option_def : option_def > +{ > + color_option_def (const char *long_option_, > + ui_file_style::color *(*get_var_address_cb_) (Context *), > + show_value_ftype *show_cmd_cb_, > + const char *set_doc_, > + const char *show_doc_ = nullptr, > + const char *help_doc_ = nullptr) > + : option_def (long_option_, var_color, > + (erased_get_var_address_ftype *) get_var_address_cb_, > + show_cmd_cb_, > + set_doc_, show_doc_, help_doc_) > + { > + var_address.color = detail::get_var_address; > + } > +}; > + > /* A group of options that all share the same context pointer to pass > to the options' get-current-value callbacks. */ > struct option_def_group > diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c > index 139ebaf8323..5e4d0355ec5 100644 > --- a/gdb/cli/cli-setshow.c > +++ b/gdb/cli/cli-setshow.c > @@ -297,6 +297,34 @@ parse_cli_var_enum (const char **args, const char *const *enums) > return match; > } > > +/* See cli-setshow.h. */ > + > +bool > +print_color_str (char *s, size_t buf_size, const ui_file_style::color &color) > +{ > + if (!buf_size) > + return false; > + > + int n; > + if (!color.is_simple ()) > + { > + uint8_t rgb[3]; > + color.get_rgb (rgb); > + n = snprintf (s, buf_size, "#%02X%02X%02X", rgb[0], rgb[1], rgb[2]); > + } > + else if (color.is_none () || color.is_basic ()) > + n = snprintf (s, buf_size, "%s", basic_color_name (color.get_value())); > + else > + n = snprintf (s, buf_size, "%d", color.get_value ()); > + > + if (n >= 0 && n < buf_size) > + return true; > + > + n = std::clamp (n, 0, buf_size - 1); > + s[n] = 0; > + return true; > +} > + > /* Do a "set" command. ARG is NULL if no argument, or the > text of the argument, and FROM_TTY is nonzero if this command is > being entered directly by the user (i.e. these are just like any > @@ -454,6 +482,12 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) > option_changed = c->var->set (match); > } > break; > + case var_color: > + { > + ui_file_style::color color = parse_var_color (arg); > + option_changed = c->var->set (color); > + } > + break; > case var_zuinteger_unlimited: > option_changed = c->var->set > (parse_cli_var_zuinteger_unlimited (&arg, true)); > @@ -535,6 +569,17 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) > gdb::observers::command_param_changed.notify > (name, c->var->get ()); > break; > + case var_color: > + { > + char s[64]; > + const ui_file_style::color &color > + = c->var->get (); > + print_color_str (s, sizeof s, color); > + > + gdb::observers::command_param_changed.notify > + (name, s); > + } > + break; > case var_boolean: > { > const char *opt = c->var->get () ? "on" : "off"; > @@ -602,6 +647,14 @@ get_setshow_command_value_string (const setting &var) > stb.puts (value); > } > break; > + case var_color: > + { > + char s[64]; > + const ui_file_style::color &value = var.get (); > + print_color_str (s, sizeof s, value); > + stb.puts (s); Could make use of ui_file_style::color::to_string () here. > + } > + break; > case var_boolean: > stb.puts (var.get () ? "on" : "off"); > break; > diff --git a/gdb/cli/cli-setshow.h b/gdb/cli/cli-setshow.h > index 3e5d105eab7..3f50cf25263 100644 > --- a/gdb/cli/cli-setshow.h > +++ b/gdb/cli/cli-setshow.h > @@ -52,6 +52,12 @@ extern int parse_cli_var_zuinteger_unlimited (const char **arg, > const char *parse_cli_var_enum (const char **args, > const char *const *enums); > > +/* Prints text representation of COLOR into S up to BUF_SIZE characters. > + Returns true if value have been written without truncation, > + false otherwise. */ > +bool > +print_color_str (char *s, size_t buf_size, const ui_file_style::color &color); > + This should be extern, and not place the function name on a new line. But I think this should go completely and the body move into a new to_string function as I mention above. > extern void do_set_command (const char *arg, int from_tty, > struct cmd_list_element *c); > extern void do_show_command (const char *arg, int from_tty, > diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c > index abf685561fa..3123a391763 100644 > --- a/gdb/cli/cli-style.c > +++ b/gdb/cli/cli-style.c > @@ -43,20 +43,6 @@ bool source_styling = true; > > bool disassembler_styling = true; > > -/* Name of colors; must correspond to ui_file_style::basic_color. */ > -static const char * const cli_colors[] = { > - "none", > - "black", > - "red", > - "green", > - "yellow", > - "blue", > - "magenta", > - "cyan", > - "white", > - nullptr > -}; > - > /* Names of intensities; must correspond to > ui_file_style::intensity. */ > static const char * const cli_intensities[] = { > @@ -132,8 +118,8 @@ cli_style_option::cli_style_option (const char *name, > ui_file_style::intensity intensity) > : changed (name), > m_name (name), > - m_foreground (cli_colors[fg - ui_file_style::NONE]), > - m_background (cli_colors[0]), > + m_foreground (fg), > + m_background (ui_file_style::NONE), > m_intensity (cli_intensities[intensity]) > { > } > @@ -144,32 +130,17 @@ cli_style_option::cli_style_option (const char *name, > ui_file_style::intensity i) > : changed (name), > m_name (name), > - m_foreground (cli_colors[0]), > - m_background (cli_colors[0]), > + m_foreground (ui_file_style::NONE), > + m_background (ui_file_style::NONE), > m_intensity (cli_intensities[i]) > { > } > > -/* Return the color number corresponding to COLOR. */ > - > -static int > -color_number (const char *color) > -{ > - for (int i = 0; i < ARRAY_SIZE (cli_colors); ++i) > - { > - if (color == cli_colors[i]) > - return i - 1; > - } > - gdb_assert_not_reached ("color not found"); > -} > - > /* See cli-style.h. */ > > ui_file_style > cli_style_option::style () const > { > - int fg = color_number (m_foreground); > - int bg = color_number (m_background); > ui_file_style::intensity intensity = ui_file_style::NORMAL; > > for (int i = 0; i < ARRAY_SIZE (cli_intensities); ++i) > @@ -181,7 +152,7 @@ cli_style_option::style () const > } > } > > - return ui_file_style (fg, bg, intensity); > + return ui_file_style (m_foreground, m_background, intensity); > } > > /* See cli-style.h. */ > @@ -254,9 +225,8 @@ cli_style_option::add_setshow_commands (enum command_class theclass, > > set_show_commands commands; > > - commands = add_setshow_enum_cmd > - ("foreground", theclass, cli_colors, > - &m_foreground, > + commands = add_setshow_color_cmd > + ("foreground", theclass, &m_foreground, > _("Set the foreground color for this property."), > _("Show the foreground color for this property."), > nullptr, > @@ -266,9 +236,8 @@ cli_style_option::add_setshow_commands (enum command_class theclass, > commands.set->set_context (this); > commands.show->set_context (this); > > - commands = add_setshow_enum_cmd > - ("background", theclass, cli_colors, > - &m_background, > + commands = add_setshow_color_cmd > + ("background", theclass, &m_background, > _("Set the background color for this property."), > _("Show the background color for this property."), > nullptr, > diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h > index 4090cf01333..4db86b00359 100644 > --- a/gdb/cli/cli-style.h > +++ b/gdb/cli/cli-style.h > @@ -67,9 +67,9 @@ class cli_style_option > const char *m_name; > > /* The foreground. */ > - const char *m_foreground; > + ui_file_style::color m_foreground; > /* The background. */ > - const char *m_background; > + ui_file_style::color m_background; > /* The intensity. */ > const char *m_intensity; > > diff --git a/gdb/command.h b/gdb/command.h > index d901da3c8cb..691c3999d78 100644 > --- a/gdb/command.h > +++ b/gdb/command.h > @@ -119,7 +119,9 @@ enum var_types > /* Enumerated type. Can only have one of the specified values. > *VAR is a char pointer to the name of the element that we > find. */ > - var_enum > + var_enum, > + /* Color type. *VAR is a ui_file_style::color structure. */ Two spaces after a '.' in comments, including the final '.'. > + var_color > }; > > /* Return true if a setting of type VAR_TYPE is backed with type T. > @@ -179,6 +181,14 @@ inline bool var_type_uses (var_types t) > return t == var_enum; > } > > +/* Return true if a setting of type T is backed by an ui_file_style::color > + variable. */ > +template<> > +inline bool var_type_uses (var_types t) > +{ > + return t == var_color; > +} > + > template struct setting_func_types_1; > > template > @@ -665,6 +675,20 @@ extern set_show_commands add_setshow_enum_cmd > setting_func_types::get get_func, show_value_ftype *show_func, > cmd_list_element **set_list, cmd_list_element **show_list); > > +extern set_show_commands add_setshow_color_cmd > + (const char *name, command_class theclass, ui_file_style::color *var, > + const char *set_doc, const char *show_doc, const char *help_doc, > + cmd_func_ftype *set_func, show_value_ftype *show_func, > + cmd_list_element **set_list, cmd_list_element **show_list); > + > +extern set_show_commands add_setshow_color_cmd > + (const char *name, command_class theclass, > + const char *set_doc, const char *show_doc, const char *help_doc, > + setting_func_types::set set_func, > + setting_func_types::get get_func, > + show_value_ftype *show_func, cmd_list_element **set_list, > + cmd_list_element **show_list); > + > extern set_show_commands add_setshow_auto_boolean_cmd > (const char *name, command_class theclass, auto_boolean *var, > const char *set_doc, const char *show_doc, const char *help_doc, > diff --git a/gdb/guile/scm-param.c b/gdb/guile/scm-param.c > index 54c8c27301a..c7ef3ddd03b 100644 > --- a/gdb/guile/scm-param.c > +++ b/gdb/guile/scm-param.c > @@ -48,6 +48,9 @@ union pascm_variable > > /* Hold a string, for enums. */ > const char *cstringval; > + > + /* Hold a color. */ > + ui_file_style::color color; > }; > > /* A GDB parameter. > @@ -193,7 +196,7 @@ pascm_make_param_smob (void) > scm_gc_malloc (sizeof (param_smob), param_smob_name); > SCM p_scm; > > - memset (p_smob, 0, sizeof (*p_smob)); > + memset (reinterpret_cast (p_smob), 0, sizeof (*p_smob)); This seems a bit random for this patch. If this is needed then this should probably be in a separate patch, with its own justification. > p_smob->cmd_class = no_class; > p_smob->type = var_boolean; /* ARI: var_boolean */ > p_smob->set_func = SCM_BOOL_F; > @@ -466,6 +469,13 @@ add_setshow_generic (enum var_types param_type, enum command_class cmd_class, > set_list, show_list); > break; > > + case var_color: > + commands = add_setshow_color_cmd (cmd_name, cmd_class, &self->value.color, > + set_doc, show_doc, help_doc, > + set_func, show_func, > + set_list, show_list); > + break; > + > default: > gdb_assert_not_reached ("bad param_type value"); > } > @@ -545,6 +555,7 @@ static const scheme_integer_constant parameter_types[] = > { "PARAM_OPTIONAL_FILENAME", var_optional_filename }, > { "PARAM_FILENAME", var_filename }, > { "PARAM_ENUM", var_enum }, > + { "PARAM_COLOR", var_color }, > > END_INTEGER_CONSTANTS > }; > @@ -611,6 +622,24 @@ pascm_param_value (const setting &var, int arg_pos, const char *func_name) > return gdbscm_scm_from_host_string (str, strlen (str)); > } > > + case var_color: > + { > + const ui_file_style::color &color = var.get (); > + if (color.is_none () || color.is_basic ()) > + { > + const char *str = basic_color_name (color.get_value ()); > + return gdbscm_scm_from_host_string (str, strlen (str)); > + } > + else if (color.is_simple ()) > + return scm_from_int (color.get_value ()); > + > + uint8_t rgb[3]; > + color.get_rgb (rgb); > + char s[64]; > + int n = snprintf (s, sizeof s, "#%02X%02X%02X", rgb[0], rgb[1], rgb[2]); > + return gdbscm_scm_from_host_string (s, n); > + } > + > case var_boolean: > { > if (var.get ()) > @@ -716,6 +745,44 @@ pascm_set_param_value_x (param_smob *p_smob, > break; > } > > + case var_color: > + SCM_ASSERT_TYPE (scm_is_string (value) || scm_is_integer (value), > + value, arg_pos, func_name, > + _("string or integer")); > + > + if (scm_is_integer (value)) > + { > + int i = scm_to_int (value); > + if (i < -1 || i > 255) > + gdbscm_out_of_range_error (func_name, arg_pos, value, > + _("must be in range from -1 to 255")); > + var.set (i); > + } > + else > + { > + SCM exception; > + > + gdb::unique_xmalloc_ptr string > + = gdbscm_scm_to_host_string (value, nullptr, &exception); > + if (string == nullptr) > + gdbscm_throw (exception); > + > + gdbscm_gdb_exception exc {}; > + try > + { > + ui_file_style::color color = parse_var_color (string.get ()); > + var.set (color); > + } > + catch (const gdb_exception &except) > + { > + exc = unpack (except); > + } > + > + GDBSCM_HANDLE_GDB_EXCEPTION (exc); > + } > + > + break; > + > case var_boolean: > SCM_ASSERT_TYPE (gdbscm_is_bool (value), value, arg_pos, func_name, > _("boolean")); > diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c > index 5d509ba4658..03db582a3ee 100644 > --- a/gdb/python/py-param.c > +++ b/gdb/python/py-param.c > @@ -46,6 +46,7 @@ static struct { > { "PARAM_ZUINTEGER", var_zuinteger }, > { "PARAM_ZUINTEGER_UNLIMITED", var_zuinteger_unlimited }, > { "PARAM_ENUM", var_enum }, > + { "PARAM_COLOR", var_color }, > { NULL, 0 } > }; > > @@ -70,6 +71,9 @@ union parmpy_variable > > /* Hold a string, for enums. */ > const char *cstringval; > + > + /* Hold a color. */ > + ui_file_style::color color = ui_file_style::NONE; > }; > > /* A GDB parameter. */ > @@ -108,6 +112,8 @@ make_setting (parmpy_object *s) > return setting (s->type, s->value.stringval); > else if (var_type_uses (s->type)) > return setting (s->type, &s->value.cstringval); > + else if (var_type_uses (s->type)) > + return setting (s->type, &s->value.color); > else > gdb_assert_not_reached ("unhandled var type"); > } > @@ -199,6 +205,49 @@ set_parameter_value (parmpy_object *self, PyObject *value) > break; > } > > + case var_color: > + { > + if (gdbpy_is_string (value)) > + { > + gdb::unique_xmalloc_ptr > + str (python_string_to_host_string (value)); > + if (str == NULL) > + return -1; > + try > + { > + self->value.color = parse_var_color (str.get()); > + } > + catch (const gdb_exception &except) > + { > + gdbpy_convert_exception (except); > + return -1; > + } > + } > + else if (PyLong_Check (value)) > + { > + long l; > + if (! gdb_py_int_as_long (value, &l)) > + return -1; > + if (l < -1 || l > 255) > + { > + PyErr_SetString (PyExc_RuntimeError, > + _("Range exceeded.")); > + return -1; > + } > + self->value.color = ui_file_style::color (l); > + } > + else if (value == Py_None) > + self->value.color = ui_file_style::NONE; > + else > + { > + PyErr_SetString (PyExc_RuntimeError, > + _("color arguments must be a string, an integer " > + "or None.")); > + return -1; > + } > + } > + break; > + > case var_boolean: > if (! PyBool_Check (value)) > { > @@ -637,6 +686,15 @@ add_setshow_generic (int parmclass, enum command_class cmdclass, > get_show_value, set_list, show_list); > break; > > + case var_color: > + /* Initialize the value, just in case. */ > + self->value.color = ui_file_style::NONE; > + commands = add_setshow_color_cmd (cmd_name.get (), cmdclass, > + &self->value.color, set_doc, > + show_doc, help_doc, get_set_value, > + get_show_value, set_list, show_list); > + break; > + > default: > gdb_assert_not_reached ("Unhandled parameter class."); > } > @@ -758,7 +816,8 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds) > && parmclass != var_string && parmclass != var_string_noescape > && parmclass != var_optional_filename && parmclass != var_filename > && parmclass != var_zinteger && parmclass != var_zuinteger > - && parmclass != var_zuinteger_unlimited && parmclass != var_enum) > + && parmclass != var_zuinteger_unlimited && parmclass != var_enum > + && parmclass != var_color) > { > PyErr_SetString (PyExc_RuntimeError, > _("Invalid parameter class argument.")); > @@ -779,7 +838,7 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds) > else > obj->enumeration = NULL; > obj->type = (enum var_types) parmclass; > - memset (&obj->value, 0, sizeof (obj->value)); > + memset (reinterpret_cast (&obj->value), 0, sizeof (obj->value)); > > if (var_type_uses (obj->type)) > obj->value.stringval = new std::string; > diff --git a/gdb/python/python.c b/gdb/python/python.c > index c7d4157b70c..b1caea5665d 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -484,6 +484,24 @@ gdbpy_parameter_value (const setting &var) > return host_string_to_python_string (str).release (); > } > > + case var_color: > + { > + const ui_file_style::color &color = var.get (); > + if (color.is_none () || color.is_basic ()) > + return host_string_to_python_string > + (basic_color_name (color.get_value ())).release (); > + else if (color.is_simple ()) > + return gdb_py_object_from_longest (color.get_value ()).release (); > + else > + { > + uint8_t rgb[3]; > + color.get_rgb (rgb); > + char s[64]; > + snprintf (s, sizeof s, "#%02X%02X%02X", rgb[0], rgb[1], rgb[2]); > + return host_string_to_python_string (s).release (); > + } > + } > + > case var_boolean: > { > if (var.get ()) > diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp > index 2242c5bf743..f6e71c89109 100644 > --- a/gdb/testsuite/gdb.base/style.exp > +++ b/gdb/testsuite/gdb.base/style.exp > @@ -298,6 +298,21 @@ proc run_style_tests { } { > set url [limited_style "http:.*html" file] > gdb_test "show version" "${vers}.*<$url>.*" \ > "'show version' is styled" > + > + if { $currently_disabled_style != "version" } { > + # Check that colors in styling can be set as integer and as RGB hex > + # tripplet. Check that the version string is styled in the output of > + # 'show version' according to the set colors. > + gdb_test_no_output "set style version intensity normal" > + gdb_test_no_output "set style version background 255" > + gdb_test_no_output "set style version foreground #FED210" > + gdb_test "show style version background" \ > + "The \033\\\[38;2;254;210;16;48;5;255m.*\".*version.*\".*style.*\033\\\[m background color is: 255" \ > + "Version's 256-color background style" > + gdb_test "show style version foreground" \ > + "The \033\\\[38;2;254;210;16;48;5;255m.*\".*version.*\".*style.*\033\\\[m foreground color is: #FED210" \ > + "Version's TrueColor foreground style" > + } > } > } > > diff --git a/gdb/ui-style.h b/gdb/ui-style.h > index fe1b2af611d..a1ba0ff413d 100644 > --- a/gdb/ui-style.h > +++ b/gdb/ui-style.h > @@ -73,6 +73,11 @@ struct ui_file_style > && m_blue == other.m_blue); > } > > + bool operator!= (const color &other) const > + { > + return ! (*this == other); > + } > + > bool operator< (const color &other) const > { > if (m_simple != other.m_simple) > @@ -104,10 +109,17 @@ struct ui_file_style > return m_simple && m_value >= BLACK && m_value <= WHITE; > } > > - /* Return the value of a basic color. */ > + /* Return true if this is one of the simple colors, false > + otherwise. */ > + bool is_simple () const > + { > + return m_simple; > + } > + > + /* Return the value of a simple color. */ > int get_value () const > { > - gdb_assert (is_basic ()); > + gdb_assert (is_simple ()); > return m_value; > } > > -- > 2.34.1