From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30193 invoked by alias); 30 Oct 2008 03:54:50 -0000 Received: (qmail 30181 invoked by uid 22791); 30 Oct 2008 03:54:50 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 30 Oct 2008 03:54:47 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 8E0C12A9649; Wed, 29 Oct 2008 23:54:45 -0400 (EDT) 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 xeGf4PVTG9ci; Wed, 29 Oct 2008 23:54:45 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 58B992A9640; Wed, 29 Oct 2008 23:54:45 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 29B80E7ACD; Wed, 29 Oct 2008 20:54:43 -0700 (PDT) Date: Thu, 30 Oct 2008 04:04:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: RFA: fix crash in expression evaluation Message-ID: <20081030035443.GB13387@adacore.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.2i 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: 2008-10/txt/msg00699.txt.bz2 Hi Tom, > 2008-10-02 Tom Tromey > > * value.c (coerce_array): Use check_typedef. Almost OK. Just a question... > 2008-10-02 Tom Tromey > > * gdb.base/pointers.exp: Add test. > * gdb.base/pointers.c (k, S): New typedefs. > (instance): New global. This part looks fine and is OK to commit once we have clarified my questions. > @@ -1692,11 +1692,16 @@ coerce_ref (struct value *arg) > struct value * > coerce_array (struct value *arg) > { > + struct type *type; Nit-picking. I think that the general convention here is to separate local declarations from the rest of the block with an empty line. I wonder if this is documented anywhere... > arg = coerce_ref (arg); > + type = check_typedef (value_type (arg)); > if (current_language->c_style_arrays > - && TYPE_CODE (value_type (arg)) == TYPE_CODE_ARRAY) > - arg = value_coerce_array (arg); > - if (TYPE_CODE (value_type (arg)) == TYPE_CODE_FUNC) > + && TYPE_CODE (type) == TYPE_CODE_ARRAY) > + { > + arg = value_coerce_array (arg); > + type = check_typedef (value_type (arg)); I don't think the second line is useful, is it? type should necessarily be a TYPE_CODE_PTR, if I understand value_coerce_array correctly. So it goes from being a TYPE_CODE_ARRAY to a TYPE_CODE_PTR. In neither case will it cause the check just below to be true. > + } > + if (TYPE_CODE (type) == TYPE_CODE_FUNC) > arg = value_coerce_function (arg); > return arg; > } Honestly, I think that the code is poorly written. How about using a case statement or at least a if/else if sequence. Would something like this work? struct type *type; arg = coerce_ref (arg); type = check_typedef (value_type (arg)) switch (TYPE_CODE (type)) { case TYPE_CODE_ARRAY: if (current_language->c_style_arrays) arg = value_coerce_array (arg); break; case TYPE_CODE_FUNC: arg = value_coerce_function (arg); break } return arg; What do you think? -- Joel :REVIEWMAIL: