From patchwork Wed Apr 6 23:22:32 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rodrigo Rivas X-Patchwork-Id: 90104 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 2167AB6F72 for ; Thu, 7 Apr 2011 09:22:43 +1000 (EST) Received: (qmail 28151 invoked by alias); 6 Apr 2011 23:22:42 -0000 Received: (qmail 28142 invoked by uid 22791); 6 Apr 2011 23:22:39 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-ww0-f51.google.com (HELO mail-ww0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 06 Apr 2011 23:22:34 +0000 Received: by wwf26 with SMTP id 26so2407843wwf.8 for ; Wed, 06 Apr 2011 16:22:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.216.82.68 with SMTP id n46mr179849wee.57.1302132152466; Wed, 06 Apr 2011 16:22:32 -0700 (PDT) Received: by 10.216.78.132 with HTTP; Wed, 6 Apr 2011 16:22:32 -0700 (PDT) In-Reply-To: References: <4D8A2403.5050708@redhat.com> <4D90A209.2020508@redhat.com> <4D94B85F.1030603@redhat.com> Date: Thu, 7 Apr 2011 01:22:32 +0200 Message-ID: Subject: Re: [C++0x] Range-based for statements and ADL From: Rodrigo Rivas To: Jonathan Wakely Cc: Jason Merrill , gcc-patches List Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Hi! It took some time but I finally re-wrote the patch and added a few testsuites. A few comments as usual: I've moved the array stuff into cp_parser_perform_range_for_lookup. I think that it belongs there: it's just a special lookup case . I finally agreed with Jason about calling lookup_member to check for the existence of begin/end before. Although maybe it could be done without this call, I don't like to rely on the subtle behavior of finish_class_member_access_expr when dealing with errors. This way is much easier to understand (and more obj-c++ friendly, probably). I've split cp_parser_perform_range_for_lookup into another function, cp_parser_range_for_member_function, that builds of the tree that does the member function call. I've changed a few error messages to "range-based %" or "range-based % loop", and the related test cases. Testsuites range-for2.C and range-for3.C worked, but just by chance. I've corrected them. About the new test cases, now the possibilities of compiler errors are greatly increased. I've tried to cover most of them, but there are surely many missing. Regards. --- Rodrigo diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 9ed3a1f..b040a53 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -1607,6 +1607,10 @@ static tree cp_parser_c_for (cp_parser *, tree, tree); static tree cp_parser_range_for (cp_parser *, tree, tree, tree); +static tree cp_parser_perform_range_for_lookup + (tree, tree *, tree *); +static tree cp_parser_range_for_member_function + (tree, tree); static tree cp_parser_jump_statement (cp_parser *); static void cp_parser_declaration_statement @@ -8555,14 +8559,20 @@ cp_parser_range_for (cp_parser *parser, tree scope, tree init, tree range_decl) } If RANGE_EXPR is an array: - BEGIN_EXPR = __range - END_EXPR = __range + ARRAY_SIZE(__range) + BEGIN_EXPR = __range + END_EXPR = __range + ARRAY_SIZE(__range) + Else if RANGE_EXPR has a member 'begin' or 'end': + BEGIN_EXPR = __range.begin() + END_EXPR = __range.end() Else: BEGIN_EXPR = begin(__range) END_EXPR = end(__range); - When calling begin()/end() we must use argument dependent - lookup, but always considering 'std' as an associated namespace. */ + If __range has a member 'begin' but not 'end', or vice versa, we must + still use the second alternative (it will surely fail, however). + When calling begin()/end() in the third alternative we must use + argument dependent lookup, but always considering 'std' as an associated + namespace. */ tree cp_convert_range_for (tree statement, tree range_decl, tree range_expr) @@ -8595,48 +8605,8 @@ cp_convert_range_for (tree statement, tree range_decl, tree range_expr) LOOKUP_ONLYCONVERTING); range_temp = convert_from_reference (range_temp); - - if (TREE_CODE (TREE_TYPE (range_temp)) == ARRAY_TYPE) - { - /* If RANGE_TEMP is an array we will use pointer arithmetic */ - iter_type = build_pointer_type (TREE_TYPE (TREE_TYPE (range_temp))); - begin_expr = range_temp; - end_expr - = build_binary_op (input_location, PLUS_EXPR, - range_temp, - array_type_nelts_top (TREE_TYPE (range_temp)), - 0); - } - else - { - /* If it is not an array, we must call begin(__range)/end__range() */ - VEC(tree,gc) *vec; - - begin_expr = get_identifier ("begin"); - vec = make_tree_vector (); - VEC_safe_push (tree, gc, vec, range_temp); - begin_expr = perform_koenig_lookup (begin_expr, vec, - /*include_std=*/true); - begin_expr = finish_call_expr (begin_expr, &vec, false, true, - tf_warning_or_error); - release_tree_vector (vec); - - end_expr = get_identifier ("end"); - vec = make_tree_vector (); - VEC_safe_push (tree, gc, vec, range_temp); - end_expr = perform_koenig_lookup (end_expr, vec, - /*include_std=*/true); - end_expr = finish_call_expr (end_expr, &vec, false, true, - tf_warning_or_error); - release_tree_vector (vec); - - /* The unqualified type of the __begin and __end temporaries should - * be the same as required by the multiple auto declaration */ - iter_type = cv_unqualified (TREE_TYPE (begin_expr)); - if (!same_type_p (iter_type, cv_unqualified (TREE_TYPE (end_expr)))) - error ("inconsistent begin/end types in range-based for: %qT and %qT", - TREE_TYPE (begin_expr), TREE_TYPE (end_expr)); - } + iter_type = cp_parser_perform_range_for_lookup (range_temp, + &begin_expr, &end_expr); } /* The new for initialization statement */ @@ -8680,6 +8650,143 @@ cp_convert_range_for (tree statement, tree range_decl, tree range_expr) return statement; } +/* Solves BEGIN_EXPR and END_EXPR as described in cp_convert_range_for. + We need to solve both at the same time because the method used + depends on the existence of members begin or end. + Returns the type deduced for the iterator expression. */ + +static tree +cp_parser_perform_range_for_lookup (tree range, tree *begin, tree *end) +{ + if (TREE_CODE (TREE_TYPE (range)) == ARRAY_TYPE) + { + if (!COMPLETE_TYPE_P (TREE_TYPE (range))) + { + error ("range-based % expression must have complete type"); + *begin = *end = error_mark_node; + return error_mark_node; + } + else + { + /* If RANGE is an array we will use pointer arithmetic */ + *begin = range; + *end = build_binary_op (input_location, PLUS_EXPR, + range, + array_type_nelts_top (TREE_TYPE (range)), + 0); + return build_pointer_type (TREE_TYPE (TREE_TYPE (range))); + } + } + else + { + /* If it is not an array, we must do a bit of magic */ + tree id_begin, id_end; + tree member_begin, member_end; + + *begin = *end = error_mark_node; + + id_begin = get_identifier ("begin"); + id_end = get_identifier ("end"); + member_begin = lookup_member (TREE_TYPE (range), id_begin, + /*protect=*/2, /*want_type=*/false); + member_end = lookup_member (TREE_TYPE (range), id_end, + /*protect=*/2, /*want_type=*/false); + + if (member_begin != NULL_TREE || member_end != NULL_TREE) + /* Use the member functions */ + { + if (member_begin != NULL_TREE) + *begin = cp_parser_range_for_member_function (range, id_begin); + else + error ("range-based % expression has an % member " + "but not a %"); + + if (member_end != NULL_TREE) + *end = cp_parser_range_for_member_function (range, id_end); + else + error ("range-based % expression has a % member " + "but not an %"); + } + else + /* Use global functions with ADL */ + { + VEC(tree,gc) *vec; + vec = make_tree_vector (); + + VEC_safe_push (tree, gc, vec, range); + + member_begin = perform_koenig_lookup (id_begin, vec, + /*include_std=*/true); + *begin = finish_call_expr (member_begin, &vec, false, true, + tf_warning_or_error); + member_end = perform_koenig_lookup (id_end, vec, + /*include_std=*/true); + *end = finish_call_expr (member_end, &vec, false, true, + tf_warning_or_error); + + release_tree_vector (vec); + } + + /* Last common checks */ + if (*begin == error_mark_node || *end == error_mark_node) + /* If one of the expressions is an error do no more checks. */ + { + *begin = *end = error_mark_node; + return error_mark_node; + } + else + { + tree iter_type = cv_unqualified (TREE_TYPE (*begin)); + /* The unqualified type of the __begin and __end temporaries should + * be the same as required by the multiple auto declaration */ + if (!same_type_p (iter_type, cv_unqualified (TREE_TYPE (*end)))) + error ("inconsistent begin/end types in range-based % " + "statement: %qT and %qT", + TREE_TYPE (*begin), TREE_TYPE (*end)); + return iter_type; + } + } +} + +/* Helper function for cp_parser_perform_range_for_lookup. + Builds a tree for RANGE.IDENTIFIER(). */ + +static tree +cp_parser_range_for_member_function (tree range, tree identifier) +{ + tree member, instance, fn; + + member = finish_class_member_access_expr (range, identifier, + false, tf_warning_or_error); + if (member == error_mark_node) + return error_mark_node; + + if (TREE_CODE (member) == COMPONENT_REF) + { + instance = TREE_OPERAND (member, 0); + fn = TREE_OPERAND (member, 1); + } + else + fn = NULL_TREE; + + if (fn && BASELINK_P (fn)) + return build_new_method_call (instance, fn, + NULL, NULL, LOOKUP_NORMAL, + NULL, tf_warning_or_error); + else + { + tree res; + VEC(tree,gc) *vec; + + vec = make_tree_vector (); + res = finish_call_expr (member, &vec, + /*disallow_virtual=*/false, + /*koenig_p=*/false, + tf_warning_or_error); + release_tree_vector (vec); + return res; + } +} /* Parse an iteration-statement. @@ -8828,7 +8935,7 @@ cp_parser_for_init_statement (cp_parser* parser, tree *decl) if (cxx_dialect < cxx0x) { error_at (cp_lexer_peek_token (parser->lexer)->location, - "range-based-for loops are not allowed " + "range-based % loops are not allowed " "in C++98 mode"); *decl = error_mark_node; } diff --git a/gcc/testsuite/g++.dg/cpp0x/range-for11.C b/gcc/testsuite/g++.dg/cpp0x/range-for11.C new file mode 100644 index 0000000..d02519a --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/range-for11.C @@ -0,0 +1,40 @@ +// Test for range-based for loop +// Test the loop with a custom iterator +// with begin/end as member functions + +// { dg-do compile } +// { dg-options "-std=c++0x" } + +struct iterator +{ + int x; + explicit iterator(int v) :x(v) {} + iterator &operator ++() { ++x; return *this; } + int operator *() { return x; } + bool operator != (const iterator &o) { return x != o.x; } +}; + +namespace foo +{ + struct container + { + int min, max; + container(int a, int b) :min(a), max(b) {} + + iterator begin() + { + return iterator(min); + } + iterator end() + { + return iterator(max + 1); + } + }; +} + +int main() +{ + foo::container c(1,4); + for (int it : c) + ; +} diff --git a/gcc/testsuite/g++.dg/cpp0x/range-for12.C b/gcc/testsuite/g++.dg/cpp0x/range-for12.C new file mode 100644 index 0000000..9b405dc --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/range-for12.C @@ -0,0 +1,116 @@ +// Test for range-based for loop with templates +// and begin/end as member functions + +// { dg-do run } +// { dg-options "-std=c++0x" } + +/* Preliminary declarations */ +namespace pre +{ + struct iterator + { + int x; + explicit iterator (int v) :x(v) {} + iterator &operator ++() { ++x; return *this; } + int operator *() { return x; } + bool operator != (const iterator &o) { return x != o.x; } + }; + + struct container + { + int min, max; + container(int a, int b) :min(a), max(b) {} + iterator begin() const + { + return iterator(min); + } + iterator end() const + { + return iterator(max); + } + + }; + +} //namespace pre + +using pre::container; +extern "C" void abort(void); + +container run_me_just_once() +{ + static bool run = false; + if (run) + abort(); + run = true; + return container(1,2); +} + +/* Template with dependent expression. */ +template int test1(const T &r) +{ + int t = 0; + for (int i : r) + t += i; + return t; +} + +/* Template with non-dependent expression and dependent declaration. */ +template int test2(const container &r) +{ + int t = 0; + for (T i : r) + t += i; + return t; +} + +/* Template with non-dependent expression (array) and dependent declaration. */ +template int test2(const int (&r)[4]) +{ + int t = 0; + for (T i : r) + t += i; + return t; +} + +/* Template with non-dependent expression and auto declaration. */ +template int test3(const container &r) +{ + int t = 0; + for (auto i : r) + t += i; + return t; +} + +/* Template with non-dependent expression (array) and auto declaration. */ +template int test3(const int (&r)[4]) +{ + int t = 0; + for (auto i : r) + t += i; + return t; +} + +int main () +{ + container c(1,5); + int a[4] = {5,6,7,8}; + + for (auto x : run_me_just_once()) + ; + + if (test1 (c) != 10) + abort(); + if (test1 (a) != 26) + abort(); + + if (test2 (c) != 10) + abort(); + if (test2 (a) != 26) + abort(); + + if (test3 (c) != 10) + abort(); + if (test3 (a) != 26) + abort(); + return 0; +} diff --git a/gcc/testsuite/g++.dg/cpp0x/range-for13.C b/gcc/testsuite/g++.dg/cpp0x/range-for13.C new file mode 100644 index 0000000..6a0fb83 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/range-for13.C @@ -0,0 +1,96 @@ +// Test for errors in range-based for loops +// with member begin/end + +// { dg-do compile } +// { dg-options "-std=c++0x" } + +//These should not be used +template int *begin(T &t) +{ + T::fail; +} +template int *end(T &t) +{ + T::fail; +} + +struct container1 +{ + int *begin(); + //no end +}; + +struct container2 +{ + int *end(); + //no begin +}; + +struct container3 +{ +private: + int *begin(); // { dg-error "is private" } + int *end(); // { dg-error "is private" } +}; + +struct container4 +{ + int *begin; + int *end; +}; + +struct container5 +{ + typedef int *begin; + typedef int *end; +}; + +struct callable +{ + int *operator()(); +}; + +struct container6 +{ + callable begin; + callable end; +}; + +struct container7 +{ + static callable begin; + static callable end; +}; + +struct private_callable +{ +private: + int *operator()(); // { dg-error "is private" } +}; + +struct container8 +{ + private_callable begin; + private_callable end; +}; + +struct container9 +{ + typedef int *(*function)(); + + function begin; + static function end; +}; + +void test1() +{ + for (int x : container1()); // { dg-error "member but not" } + for (int x : container2()); // { dg-error "member but not" } + for (int x : container3()); // { dg-error "within this context" } + for (int x : container4()); // { dg-error "cannot be used as a function" } + for (int x : container5()); // { dg-error "invalid use of" } + for (int x : container6()); + for (int x : container7()); + for (int x : container8()); // { dg-error "within this context" } + for (int x : container9()); +} diff --git a/gcc/testsuite/g++.dg/cpp0x/range-for2.C b/gcc/testsuite/g++.dg/cpp0x/range-for2.C index bfab376..17eb41d 100644 --- a/gcc/testsuite/g++.dg/cpp0x/range-for2.C +++ b/gcc/testsuite/g++.dg/cpp0x/range-for2.C @@ -8,7 +8,7 @@ struct iterator { int x; - iterator(int v) :x(v) {} + explicit iterator(int v) :x(v) {} iterator &operator ++() { ++x; return *this; } int operator *() { return x; } bool operator != (const iterator &o) { return x != o.x; } @@ -36,6 +36,6 @@ namespace foo int main() { foo::container c(1,4); - for (iterator it : c) + for (int it : c) ; } diff --git a/gcc/testsuite/g++.dg/cpp0x/range-for3.C b/gcc/testsuite/g++.dg/cpp0x/range-for3.C index 947f01c..85115a3 100644 --- a/gcc/testsuite/g++.dg/cpp0x/range-for3.C +++ b/gcc/testsuite/g++.dg/cpp0x/range-for3.C @@ -8,7 +8,7 @@ struct iterator { int x; - iterator(int v) :x(v) {} + explicit iterator(int v) :x(v) {} iterator &operator ++() { ++x; return *this; } int operator *() { return x; } bool operator != (const iterator &o) { return x != o.x; } @@ -36,7 +36,7 @@ namespace std int main() { container c(1,4); - for (iterator it : c) + for (int it : c) { } } diff --git a/gcc/testsuite/g++.dg/cpp0x/range-for9.C b/gcc/testsuite/g++.dg/cpp0x/range-for9.C index 96e9cb6..c51cbf9 100644 --- a/gcc/testsuite/g++.dg/cpp0x/range-for9.C +++ b/gcc/testsuite/g++.dg/cpp0x/range-for9.C @@ -6,6 +6,6 @@ void test() { int a[] = {0,1,2}; - for (int x : a) // { dg-error "range-based-for" } + for (int x : a) // { dg-error "range-based 'for'" } ; }