Re: [PATCH v8 4/6] GDB: Allow arbitrary keywords in integer set commands

["Maciej W. Rozycki" <macro@embecosm.com>]

On Thu, 12 Jan 2023, Simon Marchi wrote:

> This is a nice cleanup I think.  I didn't want to delay this further, so
> I did the best review I could given the time I had.  I'm fine with you
> merging this (you can add my Approved-By), we can always follow up with
> more fixes.  If others would like to review it, it's always appreciated,
> but given the time it's been sitting unreviewed, I kind of doubt that
> will happen.

 Given your rationale I chose to commit this change, despite a concern 
with not using `setting', as this code is correct regardless and as you 
say we can always update it further.  See below for details.  Thank you 
for your review.

> > +	    if (value == l->use)
> > +	      {
> > +		if (l->val.has_value ())
> > +		  value = *l->val;
> > +		else
> > +		  return allocate_value (builtin_type (gdbarch)->builtin_void);
> 
> I was wondering about this line returning void.  As far as I understand,
> this isn't used today.  It would only be used if we defined a
> literal_def without a user-visible value.  And the consequence would be
> that for this setting, using $_gdb_setting("foo") would yield void for
> that setting value.

 It's actually used with the next patch and even covered in the testsuite 
(with 6/6):

    gdb_test "p \$_gdb_setting(\"print characters\")" " = void" \
	"_gdb_setting print characters default"

I didn't think it would make sense to move the implementation of this code 
path to the next patch even though this one does not make use of it.

 Overall I think it will be best if we do not expose internals and add new 
magic numbers now that we have the infrastructure that makes it possible 
to avoid.  I only made an exception following Andrew's request here: 
<https://sourceware.org/pipermail/gdb-patches/2022-June/190389.html> to 
make `set print characters' accept "0" for "unlimited" for consistency 
with `set print elements'.

> > +	if (var.type () == var_uinteger)
> > +	  return
> > +	    value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
> > +				 static_cast<const unsigned int> (value));
> > +	else
> > +	  return
> > +	    value_from_longest (builtin_type (gdbarch)->builtin_int,
> > +				static_cast<const int> (value));
> 
> Unnecessary consts in casts.

 Fixed, thanks.

> >  	if (completion != nullptr)
> >  	  {
> > -	    if (**args == '\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.  */
> > -		completion->tracker.add_completion
> > -		  (make_unique_xstrdup ("NUMBER"));
> > -		completion->tracker.add_completion
> > -		  (make_unique_xstrdup ("unlimited"));
> > -		return {};
> > -	      }
> > -	    else if (startswith ("unlimited", *args))
> > +	    if (match->extra_literals != nullptr)
> 
> The two ifs could be merged, it would help reduce the indentation a bit.

 Good catch, thanks!  Fixed.

> > -unsigned int
> > -parse_cli_var_uinteger (var_types var_type, const char **arg,
> > -			bool expression)
> > +LONGEST
> > +parse_cli_var_integer (var_types var_type, const literal_def *extra_literals,
> > +		       const char **arg, bool expression)
> 
> The function here could perhaps  take a `const setting &`, instead of var_type
> and extra_literals, which are properties of a setting.

 It is used by `parse_option' however, where no setting is involved, so 
it's not clear to me how to do that.  I could make an artificial setting I 
suppose, but I feel like it's kind of creating a problem to fit the 
solution.  Let me know if I've missed something.

> > +	if (!printed)
> > +	  {
> > +	    if (var.type () == var_uinteger)
> > +	      stb.printf ("%u", static_cast<const unsigned int> (value));
> > +	    else
> > +	      stb.printf ("%d", static_cast<const int> (value));
> 
> The consts are unnecessary here.

 Fixed, thanks.

> > @@ -106,22 +108,25 @@ enum var_types
> >      var_optional_filename,
> >      /* String which stores a filename.  (*VAR) is a std::string.  */
> >      var_filename,
> > -    /* ZeroableInteger.  *VAR is an int.  Like var_integer except
> > -       that zero really means zero.  */
> > -    var_zinteger,
> > -    /* ZeroableUnsignedInteger.  *VAR is an unsigned int.  Zero really
> > -       means zero.  */
> > -    var_zuinteger,
> > -    /* ZeroableUnsignedInteger with unlimited value.  *VAR is an int,
> > -       but its range is [0, INT_MAX].  -1 stands for unlimited and
> > -       other negative numbers are not allowed.  */
> > -    var_zuinteger_unlimited,
> >      /* 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
> >    };
> >  
> > +/* A structure describing an extra literal accepted and shown in place
> > +   of a number.  */
> > +struct literal_def
> > +  {
> > +    /* The literal to define, e.g. "unlimited".  */
> > +    const char *literal;
> > +    /* The number to substitute internally for LITERAL or VAL;
> > +       the use of this number is not allowed (unless the same as VAL).  */
> > +    LONGEST use;
> > +    /* An optional number accepted that stands for the literal.  */
> > +    gdb::optional<LONGEST> val;
> 
> Could you please add an empty line between each field?  I find that
> makes it easier to read.

 Done (`enum var_types' above seems inconsistent about it).

> > +  };
> 
> The body of the struct should have one less indentation level (curly
> braces at column 0).

 OK (again, just copied `enum var_types' style here).

> >    template<typename T>
> > -  setting (var_types var_type, T *var)
> > -    : m_var_type (var_type), m_var (var)
> > +  setting (var_types var_type, T *var, const void *extra_literals = nullptr)
> > +    : m_var_type (var_type), m_var (var), m_extra_literals (extra_literals)
> 
> Any reason why extra_literals and m_extra_literals are void pointers,
> rather than literal_def pointers?

 I guess it's been an artefact from the previous approach which used 
`erased_args'.  Updated accordingly now.

> > +/* Guile parameter types as in PARAMETER_TYPES later on.  */
> > +
> > +typedef enum param_types
> > +  {
> > +    param_boolean,
> > +    param_auto_boolean,
> > +    param_zinteger,
> > +    param_uinteger,
> > +    param_zuinteger,
> > +    param_zuinteger_unlimited,
> > +    param_string,
> > +    param_string_noescape,
> > +    param_optional_filename,
> > +    param_filename,
> > +    param_enum,
> > +  }
> > +param_types;
> 
> Just "enum param_types, no need for the typedef.  Braces at column 0.

 Fixed.

> > +static const struct
> > +  {
> > +    /* The type of the parameter.  */
> > +    enum var_types type;
> > +
> > +    /* Extra literals, such as `unlimited', accepted in lieu of a number.  */
> > +    const literal_def *extra_literals;
> > +  }
> 
> Braces at column 0 above.

 Ditto.

> > +	if (var.type () == var_uinteger)
> > +	  return scm_from_uint (static_cast<const unsigned int> (value));
> > +	else
> > +	  return scm_from_int (static_cast<const int> (value));
> 
> Unnecessary consts in casts.

 Fixed.

> > +	if (var_type == var_uinteger)
> > +	  var.set<unsigned int> (static_cast<const unsigned int> (val));
> > +	else
> > +	  var.set<int> (static_cast<const int> (val));
> 
> Unnecessary consts in casts.

 Likewise.

> > +/* Python parameter types as in PARM_CONSTANTS below.  */
> > +
> > +typedef enum param_types
> > +  {
> > +    param_boolean,
> > +    param_auto_boolean,
> > +    param_uinteger,
> > +    param_integer,
> > +    param_string,
> > +    param_string_noescape,
> > +    param_optional_filename,
> > +    param_filename,
> > +    param_zinteger,
> > +    param_zuinteger,
> > +    param_zuinteger_unlimited,
> > +    param_enum,
> > +  }
> > +param_types;
> 
> Just "enum param_types, no need for the typedef.  Braces at column 0.

 Fixed.

> I think that having two `param_types` enums (here and in the Guile
> support) technically violates the one definition rule.  Not sure if that
> matters in practice.

 There's no violation as they are defined in separate translation units 
each and both have local scope.  And so do the respective enumeration 
constants.

> > +/* Translation from Python parameters to GDB variable types.  Keep in the
> > +   same order as PARAM_TYPES due to C++'s lack of designated initializers.  */
> > +
> > +static const struct
> > +  {
> > +    /* The type of the parameter.  */
> > +    enum var_types type;
> > +
> > +    /* Extra literals, such as `unlimited', accepted in lieu of a number.  */
> > +    const literal_def *extra_literals;
> > +  }
> 
> Braces at column 0.

 Fixed.

> > +		if (strcmp (l->literal, "unlimited") == 0)
> > +		  {
> > +		    /* Compatibility hack for API brokennes.  */
> 
> brokenness

 Good catch, thanks!

> > +	if (var.type () == var_uinteger)
> > +	  return
> > +	    gdb_py_object_from_ulongest
> > +	      (static_cast<const unsigned int> (value)).release ();
> > +	else
> > +	  return
> > +	    gdb_py_object_from_longest
> > +	      (static_cast<const int> (value)).release ();
> 
> Unnecessary const in casts.

 Fixed too.

 I'm posting the final version of the patch committed, in a reply to this 
message.

  Maciej