From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 14391385E00D for ; Sun, 18 Jul 2021 15:44:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 14391385E00D Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 80C8B81941; Sun, 18 Jul 2021 15:44:34 +0000 (UTC) Date: Sun, 18 Jul 2021 15:44:29 +0000 From: Lancelot SIX To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union Message-ID: <20210718154429.27arnsgirctcjett@ubuntu.lan> References: <20210714045520.1623120-1-simon.marchi@polymtl.ca> <20210714045520.1623120-16-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210714045520.1623120-16-simon.marchi@polymtl.ca> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Sun, 18 Jul 2021 15:44:34 +0000 (UTC) X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Sun, 18 Jul 2021 15:44:39 -0000 > A note on the Guile situation: something that makes our life a little > bit complicated is that is possible to assign and read the value of a > Guile-created parameter that is not yet registered (see previous patch > that adds a test for this). It would have been much more simple to say > that this is simply not supported anymore, but that could break existing > scripts, so I don't think it is a good idea. In some cases, for example > when printing the value of a built-in parameter, we have access to a > show command and its union setting_variable. When we have an > un-registered Guile param, we don't have a show command associated to > it, but we can pass the parameter's pascm_variable union, stored in > param_smob. Because we have these two kinds of incompatible parameters, > we need two versions of the pascm_param_value function. > Hi, I believe you can create an intermediate abstraction that works for both the registered and unregistered settings. In both cases, you can use or build a cmd_list_element::setting_variable easily, and then have only one function that does the conversion to a SCM value logic. Given how a pascm_value is laid out (i.e. a union), you can create a cmd_list_element setting_variable referencing it with something like: const cmd_list_element::setting_variable v { .bool_var = ¶m_value->boolval }; This solution is not perfect either, and it forces the const qualifier of the param_value of pascm_param_value to be dropped, but this avoids code duplication which I think is good for easier maintenance. What do you think? I have built the above changes on top of your patch series (up to this patch) and tested gdb.guile/*.exp with no regression noticed. Lancelot. --- diff --git a/gdb/guile/scm-param.c b/gdb/guile/scm-param.c index 57a8c7ec68a..9dc4a5d910d 100644 --- a/gdb/guile/scm-param.c +++ b/gdb/guile/scm-param.c @@ -113,6 +113,8 @@ struct param_smob SCM containing_scm; }; +using setting_variable = cmd_list_element::setting_variable; + static const char param_smob_name[] = "gdb:parameter"; /* The tag Guile knows the param smob by. */ @@ -133,8 +135,11 @@ static SCM unlimited_keyword; static int pascm_is_valid (param_smob *); static const char *pascm_param_type_name (enum var_types type); -static SCM pascm_param_value (enum var_types type, - const pascm_variable *var, +static SCM pascm_param_value (enum var_types type, pascm_variable *var, + int arg_pos, const char *func_name); +static SCM pascm_param_value (const cmd_list_element *, + int arg_pos, const char *func_name); +static SCM pascm_param_value (enum var_types type, const setting_variable v, int arg_pos, const char *func_name); /* Administrivia for parameter smobs. */ @@ -570,79 +575,11 @@ pascm_param_type_name (enum var_types param_type) haven't been registered yet. */ static SCM -pascm_param_value (enum var_types type, const pascm_variable *param_value, +pascm_param_value (enum var_types type, pascm_variable *param_value, int arg_pos, const char *func_name) { - /* Note: We *could* support var_integer here in case someone is trying to get - the value of a Python-created parameter (which is the only place that - still supports var_integer). To further discourage its use we do not. */ - - switch (type) - { - case var_string: - case var_string_noescape: - case var_optional_filename: - case var_filename: - { - const char *str = param_value->stringval; - - if (str == NULL) - str = ""; - - return gdbscm_scm_from_host_string (str, strlen (str)); - } - - case var_enum: - { - const char *str = param_value->cstringval; - - if (str == NULL) - str = ""; - return gdbscm_scm_from_host_string (str, strlen (str)); - } - - case var_boolean: - { - if (param_value->boolval) - return SCM_BOOL_T; - else - return SCM_BOOL_F; - } - - case var_auto_boolean: - { - enum auto_boolean ab = param_value->autoboolval; - - if (ab == AUTO_BOOLEAN_TRUE) - return SCM_BOOL_T; - else if (ab == AUTO_BOOLEAN_FALSE) - return SCM_BOOL_F; - else - return auto_keyword; - } - - case var_zuinteger_unlimited: - if (param_value->intval == -1) - return unlimited_keyword; - gdb_assert (param_value->intval >= 0); - /* Fall through. */ - case var_zinteger: - return scm_from_int (param_value->intval); - - case var_uinteger: - if (param_value->uintval == UINT_MAX) - return unlimited_keyword; - /* Fall through. */ - case var_zuinteger: - return scm_from_uint (param_value->uintval); - - default: - break; - } - - return gdbscm_make_out_of_range_error (func_name, arg_pos, - scm_from_int (type), - _("program error: unhandled type")); + setting_variable v { .bool_var = ¶m_value->boolval }; + return pascm_param_value (type, v, arg_pos, func_name); } /* Return the value of a gdb parameter as a Scheme value. @@ -655,33 +592,37 @@ pascm_param_value (enum var_types type, const pascm_variable *param_value, static SCM pascm_param_value (const cmd_list_element *show_cmd, int arg_pos, const char *func_name) +{ + gdb_assert (show_cmd->type == cmd_types::show_cmd); + gdb_assert (show_cmd->var.has_value ()); + return pascm_param_value(show_cmd->var_type, *show_cmd->var, + arg_pos, func_name); +} + +/* Return the value of a gdb parameter as a Scheme value. + + contains the common code paths to handle a variable wether is has been + registered or not. */ + +static SCM +pascm_param_value (enum var_types var_type, const setting_variable var, + int arg_pos, const char *func_name) { /* Note: We *could* support var_integer here in case someone is trying to get the value of a Python-created parameter (which is the only place that still supports var_integer). To further discourage its use we do not. */ - gdb_assert (show_cmd->type == cmd_types::show_cmd); - - switch (show_cmd->var_type) + switch (var_type) { case var_string: case var_string_noescape: case var_optional_filename: case var_filename: - { - const char *str = *show_cmd->var->char_ptr_var; - - if (str == NULL) - str = ""; - - return gdbscm_scm_from_host_string (str, strlen (str)); - } - case var_enum: { - const char *str = *show_cmd->var->const_char_ptr_var; + const char *str = *var.const_char_ptr_var; - if (str == NULL) + if (str == nullptr) str = ""; return gdbscm_scm_from_host_string (str, strlen (str)); @@ -689,7 +630,7 @@ pascm_param_value (const cmd_list_element *show_cmd, case var_boolean: { - if (*show_cmd->var->bool_var) + if (*var.bool_var) return SCM_BOOL_T; else return SCM_BOOL_F; @@ -697,7 +638,7 @@ pascm_param_value (const cmd_list_element *show_cmd, case var_auto_boolean: { - enum auto_boolean ab = *show_cmd->var->auto_boolean_var; + enum auto_boolean ab = *var.auto_boolean_var; if (ab == AUTO_BOOLEAN_TRUE) return SCM_BOOL_T; @@ -708,26 +649,26 @@ pascm_param_value (const cmd_list_element *show_cmd, } case var_zuinteger_unlimited: - if (*show_cmd->var->int_var == -1) + if (*var.int_var == -1) return unlimited_keyword; - gdb_assert (*show_cmd->var->int_var >= 0); + gdb_assert (*var.int_var >= 0); /* Fall through. */ case var_zinteger: - return scm_from_int (*show_cmd->var->int_var); + return scm_from_int (*var.int_var); case var_uinteger: - if (*show_cmd->var->unsigned_int_var == UINT_MAX) + if (*var.unsigned_int_var == UINT_MAX) return unlimited_keyword; /* Fall through. */ case var_zuinteger: - return scm_from_uint (*show_cmd->var->unsigned_int_var); + return scm_from_uint (*var.unsigned_int_var); default: break; } return gdbscm_make_out_of_range_error (func_name, arg_pos, - scm_from_int (show_cmd->var_type), + scm_from_int (var_type), _("program error: unhandled type")); }