Re: [PATCHv3 1/2] gdb: Convert enum range_type to a bit field enum

[Simon Marchi <simark@simark.ca>]

On 2020-09-19 5:48 a.m., Andrew Burgess wrote:
> The expression range_type enum represents the following ideas:
>
>   - Lower bound is set to default,
>   - Upper bound is set to default,
>   - Upper bound is exclusive.
>
> There are currently 6 entries in the enum to represent the combination
> of all those ideas.
>
> In a future commit I'd like to add stride information to the range,
> this could in theory appear with any of the existing enum entries, so
> this would take us to 12 enum entries.
>
> This feels like its getting a little out of hand, so in this commit I
> switch the range_type enum over to being a flags style enum.  There's
> one entry to represent no flags being set, then 3 flags to represent
> the 3 ideas above.  Adding stride information will require adding only
> one more enum flag.
>
> I've then gone through and updated the code to handle this change.
>
> There should be no user visible changes after this commit.

I think it's a good idea.  I noted a few comments below.

> diff --git a/gdb/expression.h b/gdb/expression.h
> index 5af10f05db1..9dc598984e0 100644
> --- a/gdb/expression.h
> +++ b/gdb/expression.h
> @@ -187,20 +187,20 @@ extern void dump_prefix_expression (struct expression *, struct ui_file *);
>
>  enum range_type
>  {
> -  /* Neither the low nor the high bound was given -- so this refers to
> -     the entire available range.  */
> -  BOTH_BOUND_DEFAULT,
> +  /* This is a standard range.  Both the lower and upper bounds are
> +     defined, and the bounds are inclusive.  */
> +  RANGE_STANDARD = 0,
> +
>    /* The low bound was not given and the high bound is inclusive.  */
> -  LOW_BOUND_DEFAULT,
> +  RANGE_LOW_BOUND_DEFAULT = 1 << 0,
> +
>    /* The high bound was not given and the low bound in inclusive.  */
> -  HIGH_BOUND_DEFAULT,
> -  /* Both bounds were given and both are inclusive.  */
> -  NONE_BOUND_DEFAULT,
> -  /* The low bound was not given and the high bound is exclusive.  */
> -  NONE_BOUND_DEFAULT_EXCLUSIVE,
> -  /* Both bounds were given.  The low bound is inclusive and the high
> -     bound is exclusive.  */
> -  LOW_BOUND_DEFAULT_EXCLUSIVE,
> +  RANGE_HIGH_BOUND_DEFAULT = 1 << 1,

I don't think the comments on RANGE_LOW_BOUND_DEFAULT and
RANGE_HIGH_BOUND_DEFAULT should mention that the other bound is
inclusive anymore, since that's controlled by another flag.

> +DEF_ENUM_FLAGS_TYPE (enum range_type, range_types);

I think the type name is confusing.  I thought at first it was related
to a TYPE_CODE_RANGE `struct type`.  What about renaming to "enum
range_flag" and the enum flags type to "range_flags"?

> diff --git a/gdb/rust-exp.y b/gdb/rust-exp.y
> index db888098c4a..5111de9cf9c 100644
> --- a/gdb/rust-exp.y
> +++ b/gdb/rust-exp.y
> @@ -2492,24 +2492,29 @@ rust_parser::convert_ast_to_expression (const struct rust_op *operation,
>
>      case OP_RANGE:
>        {
> -	enum range_type kind = BOTH_BOUND_DEFAULT;
> +	enum range_type kind = (RANGE_HIGH_BOUND_DEFAULT
> +				| RANGE_LOW_BOUND_DEFAULT);
>
>  	if (operation->left.op != NULL)
>  	  {
>  	    convert_ast_to_expression (operation->left.op, top);
> -	    kind = HIGH_BOUND_DEFAULT;
> +	    kind = RANGE_HIGH_BOUND_DEFAULT;

What I understand from this code is: if the low bound (left) is provided
to the range operator, we want to clear the RANGE_LOW_BOUND_DEFAULT bit.
So I think it would be more natural to write in the typical form to
clear bits:

  kind &= ~RANGE_LOW_BOUND_DEFAULT;

I think it expresses the intent better.

You'll need to make the enum type explicitly unsigned to be able to use
the ~ operator.

Simon