Re: [RFA] Add inclusive range support for Rust

[Joel Brobecker <brobecker@adacore.com>]

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