This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

On 03/21/13 14:07, Iyer, Balaji V wrote:
Please see my response below:

-----Original Message-----
From: Aldy Hernandez []
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

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))
       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.

I see.  You have moved the diagnostic to the caller.

However, you still have not saved the location_t for giving a better diagnostic. Don't worry. I'll put this in a laundry list I'm going to keep in the wiki so we don't loose track of things. I'll take this one.

Also, if you're only setting *rank if it's previously 0, why not exit if *rank==0 upon entry?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]