From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36107 invoked by alias); 19 Feb 2016 18:50:46 -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 36092 invoked by uid 89); 19 Feb 2016 18:50:45 -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=4499, exported, scm, changelogs 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 18:50:43 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id BC8148E37C; Fri, 19 Feb 2016 18:50:42 +0000 (UTC) Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1JIofK8018675 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 19 Feb 2016 13:50:42 -0500 From: Keith Seitz Subject: Re: [PATCH v2 02/11] [PR gdb/14441] gdb: gdbtypes: change {lookup,make}_reference_type() API To: Artemiy Volkov , gdb-patches@sourceware.org References: <1450661481-31178-1-git-send-email-artemiyv@acm.org> <1453229609-20159-1-git-send-email-artemiyv@acm.org> <1453229609-20159-3-git-send-email-artemiyv@acm.org> Cc: palves@redhat.com Message-ID: <56C76401.8070900@redhat.com> Date: Fri, 19 Feb 2016 18:50: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-3-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/msg00612.txt.bz2 On 01/19/2016 10:53 AM, Artemiy Volkov wrote: > > ./Changelog: > > 2016-01-19 Artemiy Volkov > > * gdb/dwarf2read.c (read_tag_reference_type): Use > lookup_lvalue_reference_type() instead of lookup_reference_type(). > * gdb/eval.c (evaluate_subexp_standard): Likewise. > * gdb/f-exp.y: Likewise. Nit: Instead of listing a bunch of functions and "Likewise" or "Ditto," I think we're using the shorthand: (func1, func1, func3): Use blah blah. [Sadly does not extend to filenames.] > * gdb/gdbtypes.c (make_reference_type): Generalize with rvalue > reference types. > (lookup_reference_type): Generalize with rvalue reference types. > (lookup_lvalue_reference_type): New convenience wrapper for > lookup_reference_type(). > (lookup_rvalue_reference_type): Likewise. > * gdb/gdbtypes.h: Change interface for > {make,lookup}_{rvalue,}_reference_type(). I would prefer that these function be listed explicitly -- it makes searching through the changelogs a lot easier, but then I would prefer a little better description of the change, too. The entry mentions changing the interface, but it doesn't say how. > * gdb/guile/scm-type.c (gdbscm_type_reference): Use > lookup_lvalue_reference_type() instead of lookup_reference_type(). > * gdb/guile/scm-value.c (gdbscm_value_dynamic_type): Likewise. > * gdb/parse.c (follow_types): Likewise. > * gdb/python/py-type.c (typy_reference): Likewise. > (typy_lookup_type): Likewise. > * gdb/python/py-value.c (valpy_get_dynamic_type): Likewise. > (valpy_getitem): Likewise. > * gdb/python/py-xmethods.c (gdbpy_get_xmethod_result_type): > Likewise. > (gdbpy_invoke_xmethod): Likewise. Lotsa "likewise"s here! :-) > * gdb/stabsread.c: Provide extra argument to make_reference_type() > call. > * gdb/valops.c (value_ref): Use lookup_lvalue_reference_type() > instead of lookup_reference_type(). > (value_rtti_indirect_type): Likewise. > --- > diff --git a/gdb/eval.c b/gdb/eval.c > index 78ad946..729f473 100644 > --- a/gdb/eval.c > +++ b/gdb/eval.c > @@ -2789,7 +2789,7 @@ evaluate_subexp_standard (struct type *expect_type, > > if (TYPE_CODE (check_typedef (type)) != TYPE_CODE_REF) > { > - type = lookup_reference_type (type); > + type = lookup_lvalue_reference_type (type); Indentation looks off. Please double-check. > result = allocate_value (type); > } > } > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c > index f129b0e..058b77d 100644 > --- a/gdb/gdbtypes.c > +++ b/gdb/gdbtypes.c > @@ -382,17 +382,23 @@ lookup_pointer_type (struct type *type) > } > > /* Lookup a C++ `reference' to a type TYPE. TYPEPTR, if nonzero, > - points to a pointer to memory where the reference type should be > - stored. If *TYPEPTR is zero, update it to point to the reference > - type we return. We allocate new memory if needed. */ > + points to a pointer to memory where the reference type should be stored. > + If *TYPEPTR is zero, update it to point to the reference type we return. > + REFCODE denotes the kind of reference type to lookup (lvalue or rvalue > + reference). We allocate new memory if needed. */ There's a lot less here that changed than this diff would indicate. It is okay to tack on an extra sentence describing the new parameter, leaving the rest unchanged. > struct type * > -make_reference_type (struct type *type, struct type **typeptr) > +make_reference_type (struct type *type, struct type **typeptr, > + enum type_code refcode) > { > struct type *ntype; /* New type */ > + struct type **reftype; > struct type *chain; > > - ntype = TYPE_REFERENCE_TYPE (type); > + gdb_assert (refcode == TYPE_CODE_REF || refcode == TYPE_CODE_RVALUE_REF); > + > + ntype = (refcode == TYPE_CODE_REF ? TYPE_REFERENCE_TYPE (type) > + : TYPE_RVALUE_REFERENCE_TYPE (type)); >From comments in patch 1/11, I don't think this is necessary. We know it's a reference type, and TYPE_REFERENCE_TYPE will get us the reference's type, either rvalue or lvalue. > if (ntype) > { > @@ -421,7 +427,11 @@ make_reference_type (struct type *type, struct type **typeptr) > } > > TYPE_TARGET_TYPE (ntype) = type; > - TYPE_REFERENCE_TYPE (type) = ntype; > + reftype = (refcode == TYPE_CODE_REF ? &TYPE_REFERENCE_TYPE (type) > + : &TYPE_RVALUE_REFERENCE_TYPE (type)); > + > + if(!*reftype) > + *reftype = ntype; > > /* FIXME! Assume the machine has only one representation for > references, and that it matches the (only) representation for > @@ -429,10 +439,9 @@ make_reference_type (struct type *type, struct type **typeptr) > > TYPE_LENGTH (ntype) = > gdbarch_ptr_bit (get_type_arch (type)) / TARGET_CHAR_BIT; > - TYPE_CODE (ntype) = TYPE_CODE_REF; > + TYPE_CODE (ntype) = refcode; > > - if (!TYPE_REFERENCE_TYPE (type)) /* Remember it, if don't have one. */ > - TYPE_REFERENCE_TYPE (type) = ntype; > + *reftype = ntype; > > /* Update the length of all the other variants of this type. */ > chain = TYPE_CHAIN (ntype); This whole hunk is probably simplified or (largely) unnecessary if we've agreed to remove struct type.rvalue_reference_type (from patch 1/11). > @@ -449,9 +458,23 @@ make_reference_type (struct type *type, struct type **typeptr) > details. */ > > struct type * > -lookup_reference_type (struct type *type) > +lookup_reference_type (struct type *type, enum type_code refcode) > +{ > + return make_reference_type (type, (struct type **) 0, refcode); > +} > + > +/* Separate convenience functions for lvalue and rvalue references. */ > + > +struct type * > +lookup_lvalue_reference_type (struct type *type) > +{ > + return lookup_reference_type (type, TYPE_CODE_REF); > +} > + > +struct type * > +lookup_rvalue_reference_type (struct type *type) > { > - return make_reference_type (type, (struct type **) 0); > + return lookup_reference_type (type, TYPE_CODE_RVALUE_REF); > } > Our coding standard requires a comment before each function, even trivial ones. [Please don't shoot the messenger!] Additionally, if these are exported, the comment explaining the purpose of the function should be put in the header and "See description in foo.h." (or similar) should be sufficient for the comment ahead of the implementation. > /* Lookup a function type that returns type TYPE. TYPEPTR, if > diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h > index 52419b4..d9b6b9e 100644 > --- a/gdb/gdbtypes.h > +++ b/gdb/gdbtypes.h > @@ -1725,9 +1725,13 @@ extern void append_flags_type_flag (struct type *type, int bitpos, char *name); > extern void make_vector_type (struct type *array_type); > extern struct type *init_vector_type (struct type *elt_type, int n); > > -extern struct type *lookup_reference_type (struct type *); > +extern struct type *lookup_reference_type (struct type *, enum type_code); > +extern struct type *lookup_lvalue_reference_type (struct type *); > +extern struct type *lookup_rvalue_reference_type (struct type *); > > -extern struct type *make_reference_type (struct type *, struct type **); > + > +extern struct type *make_reference_type (struct type *, struct type **, > + enum type_code); > > extern struct type *make_cv_type (int, int, struct type *, struct type **); > See previous comment. > diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c > index 1cdf953..1681a77 100644 > --- a/gdb/guile/scm-value.c > +++ b/gdb/guile/scm-value.c > @@ -601,7 +601,7 @@ gdbscm_value_dynamic_type (SCM self) > if (was_pointer) > type = lookup_pointer_type (type); > else > - type = lookup_reference_type (type); > + type = lookup_lvalue_reference_type (type); Indentation? > } > } > else if (TYPE_CODE (type) == TYPE_CODE_STRUCT) > diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c > index 03cc8d9..385cb53 100644 > --- a/gdb/python/py-type.c > +++ b/gdb/python/py-type.c > @@ -827,7 +827,7 @@ typy_lookup_type (struct demangle_component *demangled, > switch (demangled_type) > { > case DEMANGLE_COMPONENT_REFERENCE: > - rtype = lookup_reference_type (type); > + rtype = lookup_lvalue_reference_type (type); > break; Indentation? > case DEMANGLE_COMPONENT_POINTER: > rtype = lookup_pointer_type (type); > diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c > index 7dba0ad..9d479ea 100644 > --- a/gdb/python/py-value.c > +++ b/gdb/python/py-value.c > @@ -376,7 +376,7 @@ valpy_get_dynamic_type (PyObject *self, void *closure) > if (was_pointer) > type = lookup_pointer_type (type); > else > - type = lookup_reference_type (type); > + type = lookup_lvalue_reference_type (type); > } > } Indentation? > else if (TYPE_CODE (type) == TYPE_CODE_STRUCT) > @@ -766,7 +766,7 @@ valpy_getitem (PyObject *self, PyObject *key) > if (TYPE_CODE (val_type) == TYPE_CODE_PTR) > res_val = value_cast (lookup_pointer_type (base_class_type), tmp); > else if (TYPE_CODE (val_type) == TYPE_CODE_REF) > - res_val = value_cast (lookup_reference_type (base_class_type), tmp); > + res_val = value_cast (lookup_lvalue_reference_type (base_class_type), tmp); > else > res_val = value_cast (base_class_type, tmp); > } Indentation? The line is also too long. See https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Column_limits . > diff --git a/gdb/stabsread.c b/gdb/stabsread.c > index 74260b7..8a50dbd 100644 > --- a/gdb/stabsread.c > +++ b/gdb/stabsread.c > @@ -1772,7 +1772,7 @@ again: > > case '&': /* Reference to another type */ > type1 = read_type (pp, objfile); > - type = make_reference_type (type1, dbx_lookup_type (typenums, objfile)); > + type = make_reference_type (type1, dbx_lookup_type (typenums, objfile), TYPE_CODE_REF); > break; > > case 'f': /* Function returning another type */ The changed line is too long. Keith