[patch] cilkplus array notation for C (clean, independent patchset, take 1)
Iyer, Balaji V
Thu Mar 21 19:08:00 GMT 2013
Please see my response below:
> -----Original Message-----
> From: Aldy Hernandez [mailto:email@example.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.
More information about the Gcc-patches