From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7451 invoked by alias); 15 Jan 2012 20:34:46 -0000 Received: (qmail 7249 invoked by uid 22791); 15 Jan 2012 20:34:45 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 15 Jan 2012 20:34:27 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0FKYQIM018047 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 15 Jan 2012 15:34:26 -0500 Received: from host2.jankratochvil.net (ovpn-116-21.ams2.redhat.com [10.36.116.21]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q0FKYKLK012014 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Sun, 15 Jan 2012 15:34:24 -0500 Date: Sun, 15 Jan 2012 21:08:00 -0000 From: Jan Kratochvil To: Sergio Durigan Junior Cc: gdb-patches@sourceware.org Subject: Re: [RFC 1/8] Language independent bits Message-ID: <20120115203420.GA18901@host2.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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 X-SW-Source: 2012-01/txt/msg00535.txt.bz2 On Sun, 15 Jan 2012 19:55:27 +0100, Sergio Durigan Junior wrote: > static int > -unk_lang_parser (void) > +unk_lang_parser (struct parser_state *ps) > { > return 1; > } > > static void > -unk_lang_error (char *msg) > +unk_lang_error (struct parser_state *ps, char *msg) > { > error (_("Attempted to parse an expression with unknown language")); > } Do you use `ps' in these two functions in some later patch? Otherwise I do not think it should be passed. It can be always passed (sure incl. updating callers and the callers of the callers) later when needed. > +static void > +initialize_expout (struct parser_state *ps, int initial_size, > + const struct language_defn *lang, > + struct gdbarch *gdbarch) > +{ > + ps->expout_size = initial_size; > + ps->expout_ptr = 0; > + ps->expout = (struct expression *) > + xmalloc (sizeof (struct expression) > + + EXP_ELEM_TO_BYTES (ps->expout_size)); That return type cast is excessive. It had meaning for K&R C where xmalloc returns PTR but that is no longer supported. There should be done s/PTR/void */ everywhere and #define PTR should be removed and #pragma GCC poison -ed. > + ps->expout->language_defn = lang; > + ps->expout->gdbarch = gdbarch; > +} [...] > -extern struct expression *expout; > -extern int expout_size; > -extern int expout_ptr; > +#define parse_gdbarch(ps) (ps->expout->gdbarch) > +#define parse_language(ps) (ps->expout->language_defn) -> #define parse_gdbarch(ps) ((ps)->expout->gdbarch) #define parse_language(ps) ((ps)->expout->language_defn) > > -#define parse_gdbarch (expout->gdbarch) > -#define parse_language (expout->language_defn) > +struct parser_state { Opening { should be on a new line. > + > + /* The expression related to this parser state. */ Indentation should be by 2, not 4 spaces. > + > + struct expression *expout; > + > + /* The size of the expression above. */ > + > + int expout_size; Memory object size does not fit into `int', it should be `size_t'. > + > + /* The number of elements already in the expression. This is used > + to know where to put new elements. */ > + > + int expout_ptr; Likewise. > +}; [...] > +/* Reallocate the `expout' pointer inside PS so that it can accommodate > + at least LENELT expression elements. This function does nothing if > + there is enough room for the elements. */ > + > +extern void increase_expout_size (struct parser_state *ps, int lenelt); Memory object size does not fit into `int', it should be `size_t'. Sorry this is not review/approval, just comments. Thanks, Jan