Hi Aldy,
Below are my responses to a couple of the things you pointed out.
Thanks,
Balaji V. Iyer.
> -----Original Message-----
> From: Aldy Hernandez [mailto:[email protected]]
> Sent: Wednesday, June 12, 2013 12:34 PM
> To: Iyer, Balaji V
> Cc: [email protected]; Jason Merrill ([email protected]);
> [email protected]
> Subject: Re: [PATCH] Cilk Plus Array Notation for C++
>
> [Jason/Richard: there are some things below I could use your feedback on.]
>
> Hi Balaji.
>
> Overall, a lot of the stuff in cp-array-notation.c looks familiar from the C
> front-
> end changes. Can't you reuse a lot of it?
I looked into trying to combine many functionality. The issue that prohibited
me was templates and extra trees. For example, IF_STMT, FOR_STMT, MODOP_EXPR,
etc are not available in C but are in C++. So, I had to add this additional
check for those. Also, if we are processing templates we have to create
different kind of trees (e.g MODOP_EXPR intead of MODIFY_EXPR).
One way to do it is to break up the places where I am using C++ specific code
and add a language hook to handle those. I tried doing that a while back and
the whole thing looked a lot messy and I would imagine it would be hard to
debug them in future (...atleast for me). This looked organized for me, even
though a few code looks repeated. Also, the function names are repeated because
they do similar things in C and C++ only thing is that the body of the function
is different.
>
> > + case ARRAY_NOTATION_REF:
> > + {
> > + tree start_index, length, stride;
> > + op1 = tsubst_non_call_postfix_expression (ARRAY_NOTATION_ARRAY
> (t),
> > + args, complain, in_decl);
> > + start_index = RECUR (ARRAY_NOTATION_START (t));
> > + length = RECUR (ARRAY_NOTATION_LENGTH (t));
> > + stride = RECUR (ARRAY_NOTATION_STRIDE (t));
> > +
> > + /* We do type-checking here for templatized array notation triplets. */
> > + if (!TREE_TYPE (start_index)
> > + || !INTEGRAL_TYPE_P (TREE_TYPE (start_index)))
> > + {
> > + error_at (loc, "start-index of array notation triplet is not an "
> > + "integer");
> > + RETURN (error_mark_node);
> > + }
> > + if (!TREE_TYPE (length) || !INTEGRAL_TYPE_P (TREE_TYPE (length)))
> > + {
> > + error_at (loc, "length of array notation triplet is not an "
> > + "integer");
> > + RETURN (error_mark_node);
> > + }
> > + if (!TREE_TYPE (stride) || !INTEGRAL_TYPE_P (TREE_TYPE (stride)))
> > + {
> > + error_at (loc, "stride of array notation triplet is not an "
> > + "integer");
> > + RETURN (error_mark_node);
> > + }
> > + if (TREE_CODE (TREE_TYPE (op1)) == FUNCTION_TYPE)
> > + {
> > + error_at (loc, "array notations cannot be used with function type");
> > + RETURN (error_mark_node);
> > + }
> > + RETURN (build_array_notation_ref (EXPR_LOCATION (t), op1,
> start_index,
> > + length, stride, TREE_TYPE (op1)));
> > + }
>
>
> You do all this type checking here, but aren't you doing the same type
> checking
> in build_array_notation_ref() which you're going to call anyway? It looks
> like
> there is some code duplication going on.
The reason why we do this second type checking here is because we don't know
what they could be when we are parsing it. For example, in:
T x,y,z
A[x:y:z]
x, y, z could be floats and that should be flagged as error, but if x, y z are
ints, then its ok. We don't know this information until we hit this spot in pt.c
>
> Also, I see you have a build_array_notation_ref() in cp/cp-array-notation.c
> and
> also in c/c-array-notation.c. Can you not implement one function that handles
> both C and C++, or at the very least reuse some of the common things?
I looked into that also, but templates got in the way.
>
> You are missing a ChangeLog entry for the above snippet.
That I will put in.
>
> > + XDELETEVEC (compare_expr);
> > + XDELETEVEC (expr_incr);
> > + XDELETEVEC (ind_init);
> > + XDELETEVEC (array_var);
> > +
> > + for (ii = 0; ii < list_size; ii++)
> > + {
> > + XDELETEVEC (count_down[ii]);
> > + XDELETEVEC (array_value[ii]);
> > + XDELETEVEC (array_stride[ii]);
> > + XDELETEVEC (array_length[ii]);
> > + XDELETEVEC (array_start[ii]);
> > + XDELETEVEC (array_ops[ii]);
> > + XDELETEVEC (array_vector[ii]);
> > + }
> > +
> > + XDELETEVEC (count_down);
> > + XDELETEVEC (array_value);
> > + XDELETEVEC (array_stride);
> > + XDELETEVEC (array_length);
> > + XDELETEVEC (array_start);
> > + XDELETEVEC (array_ops);
> > + XDELETEVEC (array_vector);
>
> I see a lot of this business going on. Perhaps one of the core
> maintainers can comment, but I would rather use an obstack, and avoid
> having to keep track of all these little buckets-- which seems rather
> error prone, and then free the obstack all in one swoop. But I'll defer
> to Richard or Jason.
>
They are temporary variables that are used to store information necessary for
expansion. To me, dynamic arrays seem to be the most straight-forward way to do
it. Changing them would involve pretty much rewriting the whole thing and thus
maybe breaking the stability. So, if it is not a huge issue, I would like to
keep the dynamic arrays. They are not being used anywhere else just inside the
function.
