From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id PfdsKCQsAGcNUgEAWB0awg (envelope-from ) for ; Fri, 04 Oct 2024 13:55:48 -0400 Authentication-Results: simark.ca; dkim=fail reason="signature verification failed" (768-bit key; unprotected) header.d=tromey.com header.i=@tromey.com header.a=rsa-sha256 header.s=default header.b=L9UHPtIQ; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 909731E355; Fri, 4 Oct 2024 13:55:48 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.5 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_INVALID,DKIM_SIGNED,MAILING_LIST_MULTI,RCVD_IN_DNSWL_BLOCKED, RCVD_IN_VALIDITY_CERTIFIED,RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE, URIBL_BLOCKED,URIBL_DBL_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=4.0.0 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 548161E05C for ; Fri, 4 Oct 2024 13:55:47 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C7F9A385332C for ; Fri, 4 Oct 2024 17:55:46 +0000 (GMT) Received: from omta034.useast.a.cloudfilter.net (omta034.useast.a.cloudfilter.net [44.202.169.33]) by sourceware.org (Postfix) with ESMTPS id E7D5B385EC19 for ; Fri, 4 Oct 2024 17:55:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E7D5B385EC19 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E7D5B385EC19 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=44.202.169.33 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728064523; cv=none; b=epSeNMDsyL6JETvmYCUYwEZC5ASGoMn3U9UJdjcKPFgo22+mL6zdyDEr5bKYNtg4C5EEdDtlFs/suOkldkumrimfflOo5ERLNjt1kGMtmxNqOgR7mvPVr4ma+4Ey/LpwiaU7M2Xt1VvHdx1Pl+8XEeMbKuXCwio61UvBAfJ6a98= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728064523; c=relaxed/simple; bh=GH7G3atWTkY/VSL8a1JaRy/3a2u3ErclE6QRmKWXF3E=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=sKd/p70G48wSrcpSLomaKC21MkMna0q0JkwUEgTbTbdijcFr75FZoXYiHRpE1p916v5aTzHsiZbbhHJ3/0RESRQylTVDy1uVNv5xwtLIrL2/DB/32PzqJxS2+oIvbPxlmkD6MGHR+93Kwh+2CUZkeWxPSZof0+hRciemKzfkonk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from eig-obgw-6007a.ext.cloudfilter.net ([10.0.30.247]) by cmsmtp with ESMTPS id wkXKsiJ4m1zuHwmWMsXzjS; Fri, 04 Oct 2024 17:55:18 +0000 Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTPS id wmWLsyDoR0HwOwmWLsoBiw; Fri, 04 Oct 2024 17:55:17 +0000 X-Authority-Analysis: v=2.4 cv=HtNwGVTS c=1 sm=1 tr=0 ts=67002c05 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=DAUX931o1VcA:10 a=ItBw4LHWJt0A:10 a=iRWcD9KjcfK2VWckdJMA:9 a=6Ogn3jAGHLSNbaov7Orx:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:Date:References:In-Reply-To :Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=VAWTziQerroaTClditdJjPTcN0gM8FwepPjvCeMGmVE=; b=L9UHPtIQjkkqVJCjIaZRUrlvhw vptLu6rhsG16GYamG83x9sGLD3Yg1v2UKUZsJM7anGP8hpQKEIc7mRHCwAHDirLAyzBozhrC0Fz+L Ahm/lPIoj3ZL/Fj2661+K5+0+; Received: from 97-122-122-36.hlrn.qwest.net ([97.122.122.36]:43450 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96.2) (envelope-from ) id 1swmWK-002002-2M; Fri, 04 Oct 2024 11:55:16 -0600 From: Tom Tromey To: Andrei Pikas Cc: tom@tromey.com, gdb-patches@sourceware.org Subject: Re: [PATCH v7] Add an option with a color type. In-Reply-To: <20240914190452.423367-1-gdb@mail.api.win> (Andrei Pikas's message of "Sat, 14 Sep 2024 22:04:52 +0300") References: <87mskb1f83.fsf@tromey.com> <20240914190452.423367-1-gdb@mail.api.win> X-Attribution: Tom Date: Fri, 04 Oct 2024 11:55:15 -0600 Message-ID: <87r08vhhx8.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 97.122.122.36 X-Source-L: No X-Exim-ID: 1swmWK-002002-2M X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 97-122-122-36.hlrn.qwest.net (murgatroyd) [97.122.122.36]:43450 X-Source-Auth: tom+tromey.com X-Email-Count: 2 X-Org: HG=bhshared;ORG=bluehost; X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfFJ9W7xcjCk4nrQRc1PzpIwG6jMbtsa/2E/+WIawPXxfcrkI0adDaLzWAfkcOqQEqJY5IlD0fMAQT4jXwG33CpNEvyI1acFMw2HhOMhO8ow/iGEj1t7+ eu/ABcyGvQ2zOKli6HuXI7Cu/hor/ycwOkfR9EtS8osDYN8rhW54EQ0OW7+4cguqqJFKQ97nIWM8NLiv0ce5HpWzmfr7nyrAWRU= X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org >>>>> "Andrei" == Andrei Pikas 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 ().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& 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 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 & Andrei> +colorsupport () Andrei> +{ Andrei> + static const std::vector value = [] I didn't realize you could omit the '()' from a lambda. Andrei> + { Andrei> + std::vector 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 (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