From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6866 invoked by alias); 18 Dec 2013 15:59:13 -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 6856 invoked by uid 89); 18 Dec 2013 15:59:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mga02.intel.com Received: from mga02.intel.com (HELO mga02.intel.com) (134.134.136.20) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Dec 2013 15:59:11 +0000 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 18 Dec 2013 07:58:54 -0800 X-ExtLoop1: 1 Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga002.jf.intel.com with ESMTP; 18 Dec 2013 07:58:31 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.215]) by IRSMSX101.ger.corp.intel.com ([163.33.3.153]) with mapi id 14.03.0123.003; Wed, 18 Dec 2013 15:57:45 +0000 From: "Agovic, Sanimir" To: 'Joel Brobecker' CC: "tromey@redhat.com" , "palves@redhat.com" , "xdje42@gmail.com" , "gdb-patches@sourceware.org" , "Boell, Keven" Subject: RE: [PATCH v4 01/13] vla: introduce new bound type abstraction adapt uses Date: Wed, 18 Dec 2013 15:59:00 -0000 Message-ID: <0377C58828D86C4588AEEC42FC3B85A7176CB92F@IRSMSX105.ger.corp.intel.com> References: <1387282678-3847-1-git-send-email-sanimir.agovic@intel.com> <1387282678-3847-2-git-send-email-sanimir.agovic@intel.com> <20131218032427.GD3493@adacore.com> In-Reply-To: <20131218032427.GD3493@adacore.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2013-12/txt/msg00705.txt.bz2 Thanks for your review. > -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourcewa= re.org] On Behalf > Of Joel Brobecker > Sent: Wednesday, December 18, 2013 04:24 AM > To: Agovic, Sanimir > Cc: tromey@redhat.com; palves@redhat.com; xdje42@gmail.com; gdb-patches@s= ourceware.org; > Boell, Keven > Subject: Re: [PATCH v4 01/13] vla: introduce new bound type abstraction a= dapt uses >=20 > > * dwarf2read.c (read_subrange_type): Use struct bound_prop for > > declaring high/low bounds and change uses accordingly. Call > > create_range_type_1 instead of create_range_type. > > * gdbtypes.c (create_range_type_1): New function. > > (create_range_type): Convert bounds into struct bound_prop and pass > > them to create_range_type_1. > > * gdbtypes.h (struct bound_prop): New struct. > > (create_range_type_1): New function prototype. > > (struct range_bounds): Use struct bound_prop instead of LONGEST for > > high/low bounds. Remove low_undefined/high_undefined and adapt all use= s. > > (TYPE_LOW_BOUND,TYPE_HIGH_BOUND): Adapt macros to refer to the static > > part of the bound. > > * parse.c (follow_types): Set high bound kind to BOUND_UNDEFINED. >=20 > Just a suggestion, which you may choose to ignore. >=20 > I think that the _1 suffix is usually used when the function performs > the private portion of a more public routine. But in this case, > create_range_type_1 is meant to be a public routine, and the _1 > suffix is not very explicit. IMO, what would be ideal would be to > rename the current create_range_type into "create_static_range_type", > and then make create_range_type_1 the new create_range_type. I checked > the GDB tree, and there aren't that many calls to update. If people > prefer, I can even take care of that myself once the patche series > has gone in. Otherwise, another compromise solution is to rename > create_range_type_1 to create_range_type_full (for instance). >=20 Sounds good to me. I will prepend a patch doing the=20 create_range_type -> create_static_range_type thingy and use create_range_t= ype in this patch instead of create_range_type_1. > > +/* Used to store a dynamic property. */ > > + > > +struct dynamic_prop > > +{ > > + /* Determine which field of the union dynamic_prop.data is used. */ > > + enum > > + { > > + PROP_UNDEFINED, > > + PROP_CONST, > > + PROP_LOCEXPR, > > + PROP_LOCLIST > > + } kind; > > + > > + /* Storage for dynamic or static value. */ > > + union data > > + { > > + LONGEST const_val; > > + void *baton; > > + } data; >=20 > Would you mind documenting each enumeration and union field? >=20 Definitely, wired that I missed it. Thanks. > > +#define TYPE_LOW_BOUND(range_type) \ > > + TYPE_RANGE_DATA(range_type)->low.data.const_val > > +#define TYPE_HIGH_BOUND(range_type) \ > > + TYPE_RANGE_DATA(range_type)->high.data.const_val > > #define TYPE_LOW_BOUND_UNDEFINED(range_type) \ > > - TYPE_RANGE_DATA(range_type)->low_undefined > > + (TYPE_RANGE_DATA(range_type)->low.kind =3D=3D PROP_UNDEFINED) > > #define TYPE_HIGH_BOUND_UNDEFINED(range_type) \ > > - TYPE_RANGE_DATA(range_type)->high_undefined > > + (TYPE_RANGE_DATA(range_type)->high.kind =3D=3D PROP_UNDEFINED) > > +#define TYPE_HIGH_BOUND_KIND(range_type) \ > > + TYPE_RANGE_DATA(range_type)->high.kind > > +#define TYPE_LOW_BOUND_KIND(range_type) \ > > + TYPE_RANGE_DATA(range_type)->low.kind >=20 > For the record, I considered the idea of adding asserts in there, > in order to get an internal error instead of an odd bug when accessing > the wrong field. > Indeed, I have spent some time debugging just to figure out I passed the wrong "type" to the macros. > But this requires us making these macros read-only > accessors, rather than read-write. A quick experiment showed that > some units are using them to write some fields, and so we would need > to audit that first. It's a desirable change on its own, IMO, regardless > of whether we thinking adding the assert is desirable or not, but I don't > want to put the burden on this patch series, which seems already quite > sizeable on its own already. >=20 I agree here as well but I`d like to have this kind of refactoring's separa= ted from the patch series. Once the initial vla support is in I can have a clos= er look at these macros. > -- > Joel Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052