[patch] cilkplus array notation for C (clean, independent patchset, take 1)

Iyer, Balaji V balaji.v.iyer@intel.com
Thu Mar 21 19:08:00 GMT 2013


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 patchset,
> take 1)
> 
> 
> > I have found some little nits that I will point out in a reply to this
> > message.
> 
> Balaji, in Joseph's last review he mentioned:
> 
> > 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.
> 
> I see you have dispensed with the rank mismatch error altogether.  Was this on
> purpose?  For example, you now have:
> 
> >       for (ii_tree = array;
> > 	   ii_tree && TREE_CODE (ii_tree) == ARRAY_NOTATION_REF;
> > 	   ii_tree = ARRAY_NOTATION_ARRAY (ii_tree))
> > 	current_rank++;
> >       if (*rank == 0)
> > 	*rank = current_rank;
> 
> Which is basically failing to set *rank when it's already set (with no error).  Is this
> on purpose?  If so, can you explain?
> 
> 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.
> 
> 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 mismatch
> 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 is unacceptable). Also, if they both have array notations and there is a rank mismatch, then it is pointed out in line 490 of c-array-notation.c. 

> 
> Aldy



More information about the Gcc-patches mailing list