From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16506 invoked by alias); 18 Dec 2013 03:24:36 -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 16496 invoked by uid 89); 18 Dec 2013 03:24:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 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 (AES256-SHA encrypted) ESMTPS; Wed, 18 Dec 2013 03:24:34 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id A926B11675F; Tue, 17 Dec 2013 22:25:12 -0500 (EST) 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 KR5nupEbkbM5; Tue, 17 Dec 2013 22:25:12 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 1A79D116752; Tue, 17 Dec 2013 22:25:12 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9C0AEE18B1; Wed, 18 Dec 2013 07:24:27 +0400 (RET) Date: Wed, 18 Dec 2013 03:24:00 -0000 From: Joel Brobecker To: Sanimir Agovic Cc: tromey@redhat.com, palves@redhat.com, xdje42@gmail.com, gdb-patches@sourceware.org, keven.boell@intel.com Subject: Re: [PATCH v4 01/13] vla: introduce new bound type abstraction adapt uses Message-ID: <20131218032427.GD3493@adacore.com> References: <1387282678-3847-1-git-send-email-sanimir.agovic@intel.com> <1387282678-3847-2-git-send-email-sanimir.agovic@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1387282678-3847-2-git-send-email-sanimir.agovic@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-12/txt/msg00682.txt.bz2 > The rational behind this patch is to get started to implement the feature > described in dwarf4 standard (2.19) Static and Dynamic Values of Attributes. > It adds new BOUND_PROP to store either a constant, exprloc, or reference to > describe an upper-/lower bound of a subrange. Other than that no new features > are introduce. > > 2013-10-18 Sanimir Agovic > Keven Boell > > * 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 uses. > (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. Just a suggestion, which you may choose to ignore. 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). > +/* 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; Would you mind documenting each enumeration and union field? > +#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 == PROP_UNDEFINED) > #define TYPE_HIGH_BOUND_UNDEFINED(range_type) \ > - TYPE_RANGE_DATA(range_type)->high_undefined > + (TYPE_RANGE_DATA(range_type)->high.kind == 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 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. 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. -- Joel