From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9121 invoked by alias); 21 Mar 2013 19:08:06 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 8971 invoked by uid 89); 21 Mar 2013 19:07:58 -0000 X-Spam-SWARE-Status: No, score=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD autolearn=ham version=3.3.1 Received: from mga14.intel.com (HELO mga14.intel.com) (143.182.124.37) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 21 Mar 2013 19:07:55 +0000 Received: from azsmga001.ch.intel.com ([10.2.17.19]) by azsmga102.ch.intel.com with ESMTP; 21 Mar 2013 12:07:53 -0700 X-ExtLoop1: 1 Received: from fmsmsx103.amr.corp.intel.com ([10.19.9.34]) by azsmga001.ch.intel.com with ESMTP; 21 Mar 2013 12:07:53 -0700 Received: from fmsmsx102.amr.corp.intel.com ([169.254.2.241]) by FMSMSX103.amr.corp.intel.com ([169.254.3.55]) with mapi id 14.01.0355.002; Thu, 21 Mar 2013 12:07:53 -0700 From: "Iyer, Balaji V" To: Aldy Hernandez , "Joseph S. Myers" CC: gcc-patches Subject: RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1) Date: Thu, 21 Mar 2013 19:08:00 -0000 Message-ID: References: <5149D62F.9070503@redhat.com> <514B1830.5040107@redhat.com> In-Reply-To: <514B1830.5040107@redhat.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-SW-Source: 2013-03/txt/msg00825.txt.bz2 Please see my response below: > -----Original Message----- > From: Aldy Hernandez [mailto:aldyh@redhat.com] > Sent: Thursday, March 21, 2013 10:25 AM > To: Joseph S. Myers > Cc: Iyer, Balaji V; gcc-patches > Subject: Re: [patch] cilkplus array notation for C (clean, independent pa= tchset, > take 1) >=20 >=20 > > I have found some little nits that I will point out in a reply to this > > message. >=20 > Balaji, in Joseph's last review he mentioned: >=20 > > In find_rank you have error ("Rank Mismatch!"); - this is not a > > properly formatted error message according to the GNU Coding standards > > (which typically would not have any uppercase). I'd also suggest that > > when you find a rank, you store (through a location_t * pointer) the > > location of the first expression found with that rank, so if you then > > find a mismatching rank you can use error_at to point to that rank and > > then inform to point to the previous rank it didn't match. >=20 > I see you have dispensed with the rank mismatch error altogether. Was th= is on > purpose? For example, you now have: >=20 > > for (ii_tree =3D array; > > ii_tree && TREE_CODE (ii_tree) =3D=3D ARRAY_NOTATION_REF; > > ii_tree =3D ARRAY_NOTATION_ARRAY (ii_tree)) > > current_rank++; > > if (*rank =3D=3D 0) > > *rank =3D current_rank; >=20 > Which is basically failing to set *rank when it's already set (with no er= ror). Is this > on purpose? If so, can you explain? >=20 > If it's on purpose, document it in the comment at the top of the function= . And > then, why don't you exit the function immediately upon entry if *rank is = non- > zero? It's a waste of time to do the rest of the analysis if you're just= going to > throw it away. >=20 > Furthermore, Joseph suggested you store the location of the initial rank = so you > can give a meaningful error message later and tell the user where the mis= match > is occurring and where the original rank occurred. I believe I have done what Joseph mentioned. If you would look at line 473 = c-array-notation.c it mentions when LHS is scalar while RHS is not (which i= s unacceptable). Also, if they both have array notations and there is a ran= k mismatch, then it is pointed out in line 490 of c-array-notation.c.=20 >=20 > Aldy