From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28540 invoked by alias); 21 Mar 2013 16:48:37 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 26559 invoked by uid 89); 21 Mar 2013 16:48:23 -0000 X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,TW_BJ autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 21 Mar 2013 16:48:16 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1UIif0-0003LL-4J from joseph_myers@mentor.com ; Thu, 21 Mar 2013 09:48:14 -0700 Received: from SVR-IES-FEM-02.mgc.mentorg.com ([137.202.0.106]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Thu, 21 Mar 2013 09:48:13 -0700 Received: from digraph.polyomino.org.uk (137.202.0.76) by SVR-IES-FEM-02.mgc.mentorg.com (137.202.0.106) with Microsoft SMTP Server id 14.1.289.1; Thu, 21 Mar 2013 16:48:11 +0000 Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.76) (envelope-from ) id 1UIiew-0003Ou-Ef; Thu, 21 Mar 2013 16:48:10 +0000 Date: Thu, 21 Mar 2013 16:48:00 -0000 From: "Joseph S. Myers" To: Aldy Hernandez CC: "Iyer, Balaji V" , gcc-patches Subject: Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1) In-Reply-To: <5149D62F.9070503@redhat.com> Message-ID: References: <5149D62F.9070503@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2013-03/txt/msg00819.txt.bz2 Continuing the review for coding style... > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c > +extern bool contains_array_notation_expr (tree); > +extern struct c_expr fix_array_notation_expr (location_t, enum tree_code, > + struct c_expr); > +extern tree fix_conditional_array_notations (tree); > +extern tree expand_array_notation_exprs (tree); Again, include appropriate headers; no extern function declarations like this in .c files. > + error_at (c_parser_peek_token (parser)->location, > + "array notations cannot be used in declaration."); No "." at end of diagnostic. > + error_at (c_parser_peek_token (parser)->location, > + "array notations cannot be used in declaration."); Likewise. > + error_at (loc, "array notations cannot be used in a " > + "condition for a for-loop."); Likewise. > +static tree > +c_parser_array_notation (location_t loc, c_parser *parser, tree initial_index, > + tree array_value) > +{ > + c_token *token = NULL; > + tree start_index = NULL_TREE, end_index = NULL_TREE, stride = NULL_TREE; > + tree value_tree = NULL_TREE, type = NULL_TREE, array_type = NULL_TREE; > + tree array_type_domain = NULL_TREE; > + double_int x; > + > + if (!array_value || array_value == error_mark_node) Can NULL actually occur here? > + token = c_parser_peek_token (parser); > + > + if (token == NULL) c_parser_peek_token can never return NULL (EOF is a CPP_EOF token). > + error_at (loc, "start-index and length fields necessary for " > + "using array notations in pointers."); No "." at end of diagnostic. > + error_at (loc, "array notations cannot be used with function " > + "type."); Likewise. > + error_at (loc, "array notations cannot be used with " > + "function pointer arrays."); Likewise. > + error_at (loc, "start-index and length fields necessary for " > + "using array notations in dimensionless arrays."); Likewise. > + error_at (loc, "start-index and length fields necessary for " > + "using array notations in variable-length arrays."); Likewise. > + c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, NULL); > + return error_mark_node; > + } > + x = TREE_INT_CST (TYPE_MAXVAL (array_type_domain)); > + x.low++; If you want to increment a double_int value, use operator ++ on the double_int itself; don't just change the low part and ignore possible overflow. > + error_at (loc, "array notations cannot be used with function " > + "type."); No "." at end of diagnostic. > + error_at (loc, "array notations cannot be used with " > + "function pointer arrays."); Likewise. > diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c > index ddb6d39..15dc83d 100644 > --- a/gcc/c/c-typeck.c > +++ b/gcc/c/c-typeck.c > @@ -103,6 +103,8 @@ static void readonly_warning (tree, enum lvalue_use); > static int lvalue_or_else (location_t, const_tree, enum lvalue_use); > static void record_maybe_used_decl (tree); > static int comptypes_internal (const_tree, const_tree, bool *, bool *); > +extern bool contains_array_notation_expr (tree); > +extern tree find_correct_array_notation_type (tree); Include headers, don't add extern function declarations in .c files. > @@ -2303,7 +2305,17 @@ build_array_ref (location_t loc, tree array, tree index) > if (TREE_TYPE (array) == error_mark_node > || TREE_TYPE (index) == error_mark_node) > return error_mark_node; > - > + Don't change a blank line by adding trailing whitespace to it. > + error_at (loc, "rank of the array's index is greater than 1."); No "." at end of diagnostic. > + if (flag_enable_cilkplus && name > + && !strncmp (IDENTIFIER_POINTER (name), "__sec_reduce", 12)) Should replace by something cleaner than examining the name, once the way these built-ins are implemented has been changed to have enum values. > + if (flag_enable_cilkplus && fundecl && DECL_NAME (fundecl) > + && !strncmp (IDENTIFIER_POINTER (DECL_NAME (fundecl)), > + "__sec_reduce", 12)) Likewise. > + /* If array notation is used and Cilk Plus is enabled, then we do not > + worry about this error now. We will handle them in a later place. */ > + if (flag_enable_cilkplus && DECL_NAME (fundecl) > + && !strncmp (IDENTIFIER_POINTER (DECL_NAME (fundecl)), "__sec_reduce", > + 12)) Likewise. > @@ -5097,7 +5130,6 @@ convert_for_assignment (location_t location, tree type, tree rhs, > enum tree_code coder; > tree rname = NULL_TREE; > bool objc_ok = false; > - > if (errtype == ic_argpass) > { > tree selector; Avoid spurious changes like this not related to the substance of the patch. > + if (flag_enable_cilkplus && contains_array_notation_expr (cond)) > + { > + error_at (start_locus, "array notation expression cannot be used in a " > + "loop's condition"); > + return; Use %' for English apostrophes in diagnostics, so that a proper apostrophe rather than neutral vertical ' can be used in appropriate locales. > + error_at (start_locus, "array notation expression cannot be used in a " > + "loop's increment expression."); Likewise, and remove the trailing ".". -- Joseph S. Myers joseph@codesourcery.com