Hi Joseph,
Thank you very much for your response. I will look into this and get
back to you soon!
-Balaji V. Iyer.
>-----Original Message-----
>From: Joseph Myers [mailto:jos...@codesourcery.com]
>Sent: Friday, October 19, 2012 5:38 PM
>To: Iyer, Balaji V
>Cc: Richard Guenther; gcc-patches@gcc.gnu.org
>Subject: RE: [Ping]FW: [PATCH] Cilk Plus merging to trunk (2 of n)
>
>On Thu, 4 Oct 2012, Iyer, Balaji V wrote:
>
>> >>>>Here is a link to the latest spec. This should clear several of
>> >>>>the questions you are seeking.
>> >>>>(http://software.intel.com/sites/default/files/m/4/e/7/3/1/40297-
>> >>>>Intel_Cilk_plus_lang_spec_2.htm#array)
>
>This specification is much improved, especially as regards specifying the
>types of
>section expressions. I'm not convinced that "the type of the array being
>subscripted shall have a declared size" is properly defined in standard terms,
>but I
>can guess reasonable semantics - that if the array-to-pointer decay were
>considered not to occur in such a context, then the expressions for the array
>being subscripted shall have array type, not pointer type, and the array type
>shall
>not be one with unspecified size (array[]), although it may be a VLA. For
>example,
>given "int a[10];", it would be valid to say a[:] or (a)[:] but not (+a)[:].
>I don't,
>however, see any testcases at all in this patch for that particular
>requirements -
>not even for the completely clear-cut cases, such as giving an error for
>"extern int
>a[]; a[:];" or "int *a; a[:];".
>
>Say expr1 through expr9 are expressions with side effects, and you have:
>
>expr1[expr2:expr3:expr4] = expr5[expr6:expr7:expr8] + expr9;
>
>The spec says "However, in such a statement, a sub-expression with rank zero is
>evaluated only once." - that is, each of the nine expressions is evaluated
>once. I
>don't see any calls to save_expr to ensure these semantics, or any testcases
>that
>verify that they are adhered to.
>
>(Are multidimensional section expressions valid when what you have is pointers
>to pointers, e.g. "int ***p; p[0:10][0:10][0:10];"? I don't see anything to
>rule
>them out, so I assume they are valid, but don't see testcases for them either.)
>
>Looking at the patch itself:
>
>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'm not convinced that your logic, falling back to examining each operand for a
>generic expression, is correct to find the ranks of all kinds of expressions.
>For
>example, there are rules:
>
>* "The rank of a simple subscript expression (postfix-expression [ expression
>]) is
>the sum of the ranks of its operand expressions. The rank of the subscript
>operand shall not be greater than one." - how do you ensure this rule? Where
>do
>you test for errors if the subscript has too high a rank (both in the
>front-end code,
>and in the testsuite), and test (in the testsuite) for cases where the
>subscript has
>rank 1?
>
>* "The rank of a comma expression is the rank of its second operand." - I don't
>see anything special to handle that. Are there testcases for rank of comma
>expressions? Apart from testing rank, you may need to test how they are
>evaluated (that each part, with independent rank, gets fully evaluted in turn)
>- I
>don't see anything obvious in the code to handle them appropriately.
>
>In general, I'd say you should have tests in the testsuite for each syntactic
>type of
>expression supported by GCC, both standard and GNU extensions, testing how it
>interacts with section expressions - both valid cases, and cases that are
>invalid
>because of rank mismatches. As another example, you don't have tests of
>conditional expressions.
>
>Where do you test (both in code, and testcases to verify errors) that "The
>rank of
>each expression in a section triplet shall be zero."? What about "The rank of
>the
>postfix expression identifying the function to call shall be zero."? "A full
>expression shall have rank zero, unless it appears in an expression statement
>or as
>the controlling expression of an if statement."? (This means, I suppose, that
>uses
>such as initializers or sizes in array declarators must be rejected.) I'd
>advise going
>through each sentence in the relevant part of the spec that says something is
>invalid and making sure you diagnose it and have a test of this.
>
>Where in the patch you use int for the size of something (e.g. a list) on the
>host,
>please use size_t.
>
>In extract_array_notation_exprs you appear to be reallocating every time
>something is added to a list (with XRESIZEVEC). It would probably be more
>efficient to use the vec.h infrastructure for an automatically resizing vector
>on
>which you push things.
>
>In c_parser_array_notation you appear to be converting indices to
>integer_type_node in some cases, not converting at all in other cases.
>But the spec says "The expressions in a triplet are converted to ptrdiff_t.",
>so you
>need to convert to target ptrdiff_t, not target int.
>And there's a requirement that "Each of the expressions in a section triplet
>shall
>have integer type.". So you need to check that, and give an error if it
>doesn't
>have integer type, before converting - and of course add testcases for each of
>the possible positions for an expression having one that doesn't have integer
>type.
>
>In c-typeck.c you disable some errors and warnings for expressions containing
>array notations. I don't know where the later point is at which you check for
>such
>errors - but in any case, you need testcases for these diagnostics on those
>cases
>to show that they aren't being lost.
>
>In invoke.texi you have:
>
>+@opindex flag_enable_cilkplus
>
>But @opindex is for the user-visible options, not for internal variables.
>That is,
>
>@opindex fcilkplus
>
>would be appropriate.
>
>In passes.texi you refer to "the Cilk runtime library (located in libcilkrts
>directory)". But no such directory is added by this patch.
>Only add references to it in documentation with the patch that adds the
>directory.
>
>--
>Joseph S. Myers
>jos...@codesourcery.com
