From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 65108 invoked by alias); 25 Apr 2018 16:52:11 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 65096 invoked by uid 89); 25 Apr 2018 16:52:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=productions, officially X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 25 Apr 2018 16:52:09 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id BBFAE117535; Wed, 25 Apr 2018 12:52:07 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 689z1brOyLmE; Wed, 25 Apr 2018 12:52:07 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 8B14F117536; Wed, 25 Apr 2018 12:52:07 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id DD3B583055; Wed, 25 Apr 2018 09:52:05 -0700 (PDT) Date: Wed, 25 Apr 2018 16:52:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA] Add inclusive range support for Rust Message-ID: <20180425165205.cwkvthuotlpaq67z@adacore.com> References: <20180329201609.13699-1-tom@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180329201609.13699-1-tom@tromey.com> User-Agent: NeoMutt/20170113 (1.7.2) X-SW-Source: 2018-04/txt/msg00520.txt.bz2 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 > > 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) : New field. > (DOTDOTEQ): New constant. > (range_expr): Add "..=" productions. > (operator_tokens): Add "..=" token. > (ast_range): Add "inclusive" parameter. > (convert_ast_to_expression) : Handle inclusive > ranges. > * parse.c (operator_length_standard) : Handle new > bounds values. > * expression.h (enum range_type) 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