This is the mail archive of the
mailing list for the GCC project.
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
- From: Aldy Hernandez <aldyh at redhat dot com>
- To: "Joseph S. Myers" <joseph at codesourcery dot com>
- Cc: "Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 21 Mar 2013 09:24:48 -0500
- Subject: Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
- References: <5149D62F dot 9070503 at redhat dot com>
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.