From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24142 invoked by alias); 19 Feb 2016 19:01:07 -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 24127 invoked by uid 89); 19 Feb 2016 19:01:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=encouraging, mechanical, submissions, permitting X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 19 Feb 2016 19:01:04 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 1612064365; Fri, 19 Feb 2016 19:01:03 +0000 (UTC) Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1JJ11WO023203 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 19 Feb 2016 14:01:02 -0500 From: Keith Seitz Subject: Re: [PATCH v2 08/11] [PR gdb/14441] gdb: convert lvalue reference type check to general reference type check To: Artemiy Volkov References: <1450661481-31178-1-git-send-email-artemiyv@acm.org> <1453229609-20159-1-git-send-email-artemiyv@acm.org> <1453229609-20159-9-git-send-email-artemiyv@acm.org> Cc: gdb-patches@sourceware.org Message-ID: <56C7666D.9000700@redhat.com> Date: Fri, 19 Feb 2016 19:01:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <1453229609-20159-9-git-send-email-artemiyv@acm.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00618.txt.bz2 On 01/19/2016 10:53 AM, Artemiy Volkov wrote: > In almost all contexts (except for overload resolution rules and expression > semantics), lvalue and rvalue references are equivalent. That means that in > all but these cases we can replace a TYPE_CODE_REF check to TYPE_REFERENCE > check. This patch does exactly that. > This is largely a mechanical change, but I do have a few nits to point out. See below. May I also bring to your attention that any subsequent patch submissions keep purely mechanical substitutions (like the majority of this patch) separate from anything more substantial. This patch contains some non-mechanical changes, such as cleanups (valops.c:value_cast) and additional handling for TYPE_CODE_RVALUE_REF in a handful of places. As a reviewer (and it may just be that I'm not very versed at it), I find it easy to lose these in a sea of mechanical changes. > 2016-01-19 Artemiy Volkov > > * gdb/aarch64-tdep.c (aarch64_type_align): Change TYPE_CODE_REF > check to TYPE_REFERENCE check. > (aarch64_extract_return_value): Likewise. > (aarch64_store_return_value): Likewise. > * gdb/amd64-tdep.c (amd64_classify): Likewise. [big snip] We are now encouraging/permitting developers to use the phrase "Update all callers." or "All callers updated." with large, mechanical changes like this. > diff --git a/gdb/eval.c b/gdb/eval.c > index 729f473..443a08c 100644 > --- a/gdb/eval.c > +++ b/gdb/eval.c > @@ -2509,7 +2509,7 @@ evaluate_subexp_standard (struct type *expect_type, > { > type = check_typedef (value_type (arg1)); > if (TYPE_CODE (type) == TYPE_CODE_PTR > - || TYPE_CODE (type) == TYPE_CODE_REF > + || TYPE_REFERENCE (type) > /* In C you can dereference an array to get the 1st elt. */ > || TYPE_CODE (type) == TYPE_CODE_ARRAY > ) Superfluous whitespace change? > @@ -2787,9 +2787,9 @@ evaluate_subexp_standard (struct type *expect_type, > { > struct type *type = value_type (result); > > - if (TYPE_CODE (check_typedef (type)) != TYPE_CODE_REF) > + if (!TYPE_REFERENCE (type)) > { > - type = lookup_lvalue_reference_type (type); > + type = lookup_lvalue_reference_type (type); > result = allocate_value (type); > } > } Same here? > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c > index 058b77d..fd17d3c 100644 > --- a/gdb/gdbtypes.c > +++ b/gdb/gdbtypes.c > @@ -3475,10 +3475,11 @@ rank_one_type (struct type *parm, struct type *arg, struct value *value) > > /* See through references, since we can almost make non-references > references. */ > - if (TYPE_CODE (arg) == TYPE_CODE_REF) > + Superfluous whitespace. > + if (TYPE_REFERENCE (arg)) > return (sum_ranks (rank_one_type (parm, TYPE_TARGET_TYPE (arg), NULL), > REFERENCE_CONVERSION_BADNESS)); > - if (TYPE_CODE (parm) == TYPE_CODE_REF) > + if (TYPE_REFERENCE (parm)) > return (sum_ranks (rank_one_type (TYPE_TARGET_TYPE (parm), arg, NULL), > REFERENCE_CONVERSION_BADNESS)); > if (overload_debug) > diff --git a/gdb/valops.c b/gdb/valops.c > index cc01689..b4f9ba5 100644 > --- a/gdb/valops.c > +++ b/gdb/valops.c > @@ -360,24 +360,20 @@ value_cast (struct type *type, struct value *arg2) > if (value_type (arg2) == type) > return arg2; > > - code1 = TYPE_CODE (check_typedef (type)); > - > /* Check if we are casting struct reference to struct reference. */ > - if (code1 == TYPE_CODE_REF) > + if (TYPE_REFERENCE (check_typedef (type))) > { > /* We dereference type; then we recurse and finally > we generate value of the given reference. Nothing wrong with > that. */ > struct type *t1 = check_typedef (type); > struct type *dereftype = check_typedef (TYPE_TARGET_TYPE (t1)); > - struct value *val = value_cast (dereftype, arg2); > + struct value *val = value_cast (dereftype, arg2); > > return value_ref (val, TYPE_CODE (t1)); > } > > - code2 = TYPE_CODE (check_typedef (value_type (arg2))); > - > - if (code2 == TYPE_CODE_REF) > + if (TYPE_REFERENCE (check_typedef (value_type (arg2)))) > /* We deref the value and then do the cast. */ > return value_cast (type, coerce_ref (arg2)); > A bunch of cleanups like this would normally be considered a separate change. I'm not going to ask anything, but be aware that the final reviewer may call this out. > @@ -1729,14 +1725,12 @@ typecmp (int staticp, int varargs, int nargs, > char *>, and properly access map["hello"], because the > argument to [] will be a reference to a pointer to a char, > and the argument will be a pointer to a char. */ > - while (TYPE_CODE(tt1) == TYPE_CODE_REF > - || TYPE_CODE (tt1) == TYPE_CODE_PTR) > - { > + while (TYPE_REFERENCE(tt1) || TYPE_CODE (tt1) == TYPE_CODE_PTR) { > tt1 = check_typedef( TYPE_TARGET_TYPE(tt1) ); > } > while (TYPE_CODE(tt2) == TYPE_CODE_ARRAY > || TYPE_CODE(tt2) == TYPE_CODE_PTR > - || TYPE_CODE(tt2) == TYPE_CODE_REF) > + || TYPE_REFERENCE(tt2)) > { > tt2 = check_typedef (TYPE_TARGET_TYPE(tt2)); > } Formatting errors. I realize you probably just did a text replace, but since you've touched a handful of the places where there were errors, please fix those instances. "TYPE_REFERENCE (". You do not need to fix the other formatting errors here. Just the two lines you changed. Keith