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

Simon Marchi
2020-09-19T13:50:42Z

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