Re: [RFA] Add inclusive range support for Rust

Joel Brobecker
2018-04-25T16:52:11Z

On Thu, Mar 29, 2018 at 02:16:09PM -0600, Tom Tromey wrote:
> Rust recently stabilized the inclusive range feature:
>
> https://github.com/rust-lang/rust/issues/28237
>
> An inclusive range is an expression like "..= EXPR" or "EXPR ..=
> EXPR". It is like an ordinary range, except the upper bound is
> inclusive, not exclusive.
>
> This patch adds support for this feature to gdb.
>
> Regression tested on x86-64 Fedora 26.
>
> Note that review is required because this patch touches some non-Rust
> code.
>
> 2018-03-29 Tom Tromey <tom@tromey.com>
>
> PR rust/22545:
> * rust-lang.c (rust_inclusive_range_type_p): New function.
> (rust_range): Handle inclusive ranges.
> (rust_compute_range): Likewise.
> * rust-exp.y (struct rust_op) <inclusive>: New field.
> (DOTDOTEQ): New constant.
> (range_expr): Add "..=" productions.
> (operator_tokens): Add "..=" token.
> (ast_range): Add "inclusive" parameter.
> (convert_ast_to_expression) <case OP_RANGE>: Handle inclusive
> ranges.
> * parse.c (operator_length_standard) <case OP_RANGE>: Handle new
> bounds values.
> * expression.h (enum range_type) <NONE_BOUND_DEFAULT_INCLUSIVE,
> LOW_BOUND_DEFAULT_INCLUSIVE>: New constants.
> * expprint.c (print_subexp_standard): Handle new bounds values.
> (dump_subexp_body_standard): Likewise.

I'm not sure I'm competent to review, but once I understand better
the existing enums for enum range_type, I think I'll be able to
officially approve.

A couple of comments below.

> @@ -1102,12 +1109,18 @@ dump_subexp_body_standard (struct expression *exp,
> case LOW_BOUND_DEFAULT:
> fputs_filtered ("Range '..EXP'", stream);
> break;
> + case LOW_BOUND_DEFAULT_INCLUSIVE:
> + fputs_filtered ("Range '..=EXP'", stream);
> + break;
> case HIGH_BOUND_DEFAULT:
> fputs_filtered ("Range 'EXP..'", stream);
> break;
> case NONE_BOUND_DEFAULT:
> fputs_filtered ("Range 'EXP..EXP'", stream);
> break;
> + case NONE_BOUND_DEFAULT_INCLUSIVE:
> + fputs_filtered ("Range 'EXP..=EXP'", stream);
> + break;
> default:
> fputs_filtered ("Invalid Range!", stream);
> break;

This is my opinion, so please feel free to disagree:

Using the rust-like syntax in the _INCLUSIVE cases ('=EXP') can be
a bit mysterious to someone not familiar with Rust. Or is it something
that's more widespread than I thought? If you agree, I'd like to
suggest we generate the range using the standard mathematical
notations instead, so it's language-agnostic and unequivocal.
We'd be changing it for all cases so that we always know whether
the bounds are inclusive or exclusive.

> diff --git a/gdb/expression.h b/gdb/expression.h
> index 7abd7f7503..86ee698aed 100644
> --- a/gdb/expression.h
> +++ b/gdb/expression.h
> @@ -150,15 +150,18 @@ extern void dump_prefix_expression (struct expression *, struct ui_file *);
>
> /* In an OP_RANGE expression, either bound could be empty, indicating
> that its value is by default that of the corresponding bound of the
> - array or string. So we have four sorts of subrange. This
> - enumeration type is to identify this. */
> -
> + array or string. Also, the upper end of the range can be exclusive
> + or inclusive. So we have six sorts of subrange. This enumeration
> + type is to identify this. */
> +
> enum range_type
> {
> BOTH_BOUND_DEFAULT, /* "(:)" */
> LOW_BOUND_DEFAULT, /* "(:high)" */
> HIGH_BOUND_DEFAULT, /* "(low:)" */
> - NONE_BOUND_DEFAULT /* "(low:high)" */
> + NONE_BOUND_DEFAULT, /* "(low:high)" */
> + NONE_BOUND_DEFAULT_INCLUSIVE, /* Rust "low..=high" */
> + LOW_BOUND_DEFAULT_INCLUSIVE, /* Rust "..=high" */
> };

Just a note to refer to my earlier email asking about the meaning
the previously existing enums (inclusive or exclusive), and perhaps
a suggestion to adjust the documentation above to make it unequivocal
by using the mathematical notation for each and everyone of them.

--
Joel