From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9330 invoked by alias); 2 Nov 2010 19:23:26 -0000 Received: (qmail 9321 invoked by uid 22791); 2 Nov 2010 19:23:25 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,TW_CP X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 02 Nov 2010 19:23:20 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id AEED32BABC0; Tue, 2 Nov 2010 15:23:18 -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 Q1kFLN5x3In5; Tue, 2 Nov 2010 15:23:18 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 0EC7B2BABA8; Tue, 2 Nov 2010 15:23:17 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id C10ADF588F; Tue, 2 Nov 2010 12:23:11 -0700 (PDT) Date: Tue, 02 Nov 2010 19:23:00 -0000 From: Joel Brobecker To: Ken Werner Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [patch] initial OpenCL C language support Message-ID: <20101102192311.GI2492@adacore.com> References: <201010221920.30046.ken@linux.vnet.ibm.com> <20101026195747.GE2847@adacore.com> <20101026200326.GF2847@adacore.com> <201010271535.05100.ken@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201010271535.05100.ken@linux.vnet.ibm.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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: 2010-11/txt/msg00046.txt.bz2 > > Just to clarify a bit what I was trying to say: Most of the comments are > > mostly cosmetic, and I don't think we need to verify that you followed > > the comments correctly. If this was the only comments I had, I would be > > comfortable with pre-approving the patch, particularly since Tom already > > looked at it as well. But before the patch goes in, I'd like to understand > > what the reason for the changes in c-exp.y... > > The reason for looking up the primitive types instead of referring to > the builtins is that the builtin types may have a wrong type size. The > OpenCL type long for example is expected to have a size of 8 byte > while the size of the GDB builtin long is dependant on the current > architecture and might be only 4 bytes. But you have the gdbarch vector when building the OpenCL long, right? (see opencl_language_arch_info). For instance, in Ada, we don't know the size of type "Integer", so we ask the gdbarch what is the size of int. lai->primitive_type_vector [ada_primitive_type_int] = arch_integer_type (gdbarch, gdbarch_int_bit (gdbarch), 0, "integer"); That way, we can use the builtin types directly. Would that work in your case? > + /* Triple vectors have the size of a quad vector. */ ^^^ missing second space > +/* Returns non-zero if the array ARR contains duplicates within > + the first N elements. */ non-zero should be spelled nonzero. I have known that for quite a while because a friend of mine is really good at spelling, but never really thought much until I saw this being explicitly mentioned in the GCC Coding Conventions. Let's try to fix them one at a time... > + for (i = offset; i < n; i++) > + { > + memcpy (value_contents_raw (v) + j++ * elsize, > + value_contents (c->val) + c->indices[i] * elsize, > + elsize); > + } The curly braces are unnecessary in this case. > + for (i = start; i < end; i++) > + { > + int startoffset = (i == start) ? startrest : 0; > + int length = (i == end) ? endrest : elsize; > + if (!value_bits_valid (c->val, c->indices[i] * elsize + startoffset, > + length)) Missing empty line after variable declarations... > + for (i = 0; i < c->n; i++) > + { > + if (value_bits_valid (c->val, c->indices[i] * elsize, elsize)) > + return 1; > + } Unecessary curly braces... > + for (i = 0; i < n; i++) > + { > + /* Copy src val contents into the destination value. */ > + memcpy (value_contents_writeable (ret) > + + (i * TYPE_LENGTH (elm_type)), > + value_contents (val) > + + (indices[i] * TYPE_LENGTH (elm_type)), > + TYPE_LENGTH (elm_type)); > + } Likewise. > +/* Perform a relational operation on two operands. */ > +static struct value * > +opencl_relop (struct expression *exp, struct value *arg1, struct value *arg2, > + enum exp_opcode op) Missing empty line after the function description. > + if (!t1_is_vec && !t2_is_vec) > + { > + int tmp = scalar_relop (arg1, arg2, op); > + struct type *type = > + language_bool_type (exp->language_defn, exp->gdbarch); > + val = value_from_longest (type, tmp); > + } Missing empty line after variable declarations... > +# Increase timeout > +set timeout 60 > +verbose "Timeout set to $timeout seconds" 2 Have we determine why it is necessary to increase the timeout to 60? -- Joel