Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union

[Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>]

> 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 = &param_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);
 \f
 /* 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 = &param_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"));
 }