Message ID | AANLkTikoD0hoj36RqO163uXedvtxF1ntoageDv_Dzz3M@mail.gmail.com |
---|---|
State | New |
Headers | show |
On 29 March 2011 01:49, Rodrigo Rivas wrote: > Hi again. > > Here it is my first try at this. I have changed the list to > gcc-patches, I don't know if cross post would be correct. > Please, note that this patch is not finished: the new test cases are > still missing, and expect format mistakes, misspellings and the > like... > > A few comments: > > 1. I'm not sure about what should happen if the begin/end found in > class scope are not ordinary functions. My guess is that if it is a > function (static or non-static) it is called normally, and if it is a > member variable it is searched for an operator()(). If it is a type it > should fail. That is what I tried to implement (I think it works). The wording was carefully written so that if name lookup finds any "r.begin" then it will try "r.begin()" so yes, it should be able to call data members of callable type. > 2. The function cp_parser_perform_range_for_lookup() does the same job > twice, but interleaved, so it's not easy to avoid typing everything > once and again. Maybe it should be split in several pieces. > Suggestions welcome. > > 3. In the function cp_parser_perform_range_for_lookup() I made quite a > shameful, but clever, abuse of the local variables and arguments. I've > seen similar patterns in the code, but if you think it is too much, I > can rewrite it. > > 4. The approach to error diagnostics can be done in so many ways that > I cannot make my mind. I think that the attached patch does a > reasonable job, but again... Should we consistently refer to %<for%> so the keyword is highlighted? > 5. Hidden in the new wording, there are a restriction about arrays of > incomplete type or unknown size. That restriction should have been > obvious before (easy to say now ;-), and it is quite unrelated to the > other changes. But I don't know if it is worth its own patch (3 lines > an a future test). > > Awaiting for comments. > > -- > Rodrigo >
On Tue, Mar 29, 2011 at 10:12 AM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> Should we consistently refer to %<for%> so the keyword is highlighted?
Now that you say... I've not been quite consistent. We could say
"range-based %<for%>", with only a dash between 'range' and 'based'.
On Tue, Mar 29, 2011 at 4:38 AM, Rodrigo Rivas <rodrigorivascosta@gmail.com> wrote: > On Tue, Mar 29, 2011 at 10:12 AM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote: >> Should we consistently refer to %<for%> so the keyword is highlighted? > Now that you say... I've not been quite consistent. We could say > "range-based %<for%>", with only a dash between 'range' and 'based'. > or "new-style for loop"?
On Tue, Mar 29, 2011 at 5:46 PM, Gabriel Dos Reis
<gdr@integrable-solutions.net> wrote:
> or "new-style for loop"?
Well, that is what they are called in Java, isn't it? And the syntax
is just the same, so it would make kind of sense.
But in the C++0x draft and the GCC docs it is almost always called
"range-based for loops, so I think it is preferred.
Talking of this... I tried the simplest wrong range-based for, with my
latest patch:
int main()
{
for (int x : 0)
;
}
And that is what I got:
test.cpp: In function ‘int main()’:
test.cpp:3:18: error: ‘begin’ was not declared in this scope
test.cpp:3:18: error: ‘end’ was not declared in this scope
That's ok. But now, if I add #include <vector> I get:
test.cpp: In function ‘int main()’:
test.cpp:4:18: error: no matching function for call to ‘begin(int&)’
test.cpp:4:18: note: candidates are:
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:87:5:
note: template<class _Tp, unsigned int _Nm> _Tp* std::begin(_Tp
(&)[_Nm])
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:58:63:
note: template<class _Container> decltype (__cont->begin())
std::begin(const _Container&)
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:48:57:
note: template<class _Container> decltype (__cont->begin())
std::begin(_Container&)
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/initializer_list:86:38:
note: template<class _Tp> constexpr const _Tp*
std::begin(std::initializer_list<_Tp>)
test.cpp:4:18: error: no matching function for call to ‘end(int&)’
test.cpp:4:18: note: candidates are:
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:97:5:
note: template<class _Tp, unsigned int _Nm> _Tp* std::end(_Tp
(&)[_Nm])
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:78:59:
note: template<class _Container> decltype (__cont->end())
std::end(const _Container&)
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:68:53:
note: template<class _Container> decltype (__cont->end())
std::end(_Container&)
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/initializer_list:96:36:
note: template<class _Tp> constexpr const _Tp*
std::end(std::initializer_list<_Tp>)
Although the error messages are technically correct, I find them too
verbose and probably misleading to the novice: "'begin' not declared?
I'm not using 'begin', I'm using 'for').
Note also that the error could be corrected adding member functions
begin/end (if the range were of class type), but that is not suggested
in the error message.
Maybe a simpler error is better. such as:
"expression is not valid as range in a range-based %<for%> loop
because it lacks begin/end members or begin(int&) and end(int&)
overloads." (I leave the wording to someone more skilled with
English).
Regards.
--
Rodrigo
On 29 March 2011 17:41, Rodrigo Rivas wrote: > > Maybe a simpler error is better. such as: > "expression is not valid as range in a range-based %<for%> loop > because it lacks begin/end members or begin(int&) and end(int&) > overloads." (I leave the wording to someone more skilled with > English). If you can prevent the full lookup error and print something different I think users would be very grateful indeed. How about "No suitable %<begin%> and %<end%> functions found for range expression of type %qT in %<for%> statement" ? That
On Tue, Mar 29, 2011 at 8:22 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote: > How about "No suitable %<begin%> and %<end%> functions found for range > expression of type %qT in %<for%> statement" ? Nice. But the problem here is that there are a lot of different error conditions: 1. begin member but not end member. 2. end member but not begin member. 3. begin and end member but one or both are not callable/accesible/whatever. 3. no begin/end members, a begin global function but not end. 4. no begin/end members, an end global function but not begin. 5. no begin/end members, begin and end functions but one or both not callable/wrong overload/etc. 6. no begin/end members, no begin/end global functions. Since the standard headers have the std::begin/std::end templates, this will not happen when you include (most) STL headers. What is the best message for each? Does it make sense to have up to 6 different messages? Too many? I just can't tell. -- Rodrigo
On 29 March 2011 21:33, Rodrigo Rivas wrote: > On Tue, Mar 29, 2011 at 8:22 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote: >> How about "No suitable %<begin%> and %<end%> functions found for range >> expression of type %qT in %<for%> statement" ? > > Nice. > But the problem here is that there are a lot of different error conditions: > 1. begin member but not end member. > 2. end member but not begin member. I think these cases should say something like "invalid lookup results, member 'begin' and non-member 'end'" (or vice versa) as we had in last week's patch. Or maybe it would be better to say "lookup for 'for' found member 'begin' but no member 'end'" > 3. begin and end member but one or both are not callable/accesible/whatever. I would definitely want to get an "access denied", "deleted function" etc. error for that case. > 3. no begin/end members, a begin global function but not end. > 4. no begin/end members, an end global function but not begin. Maybe something similar to the text I suggested above, but only mentioning the lookup that failed, "no suitable 'begin' found for range expression ..." > 5. no begin/end members, begin and end functions but one or both not > callable/wrong overload/etc. In some cases I would probably want to know the actual error, not just "cannot use range-based for" If a begin() is found but can't be called, showing the "candidates are..." list might be helpful to see the problem e.g. if my::begin(my::type&) can't be called for const my::type. > 6. no begin/end members, no begin/end global functions. Since the > standard headers have the std::begin/std::end templates, this will not > happen when you include (most) STL headers. Maybe this is the only case where the text I suggested is useful, I think a special "can't use range-based for" makes sense in this case, instead of "begin not declared in this scope" > What is the best message for each? Does it make sense to have up to 6 > different messages? Too many? It would be nice for users, but it's certainly more work, and potentially more places for bugs to creep in.
On Tue, Mar 29, 2011 at 11:13 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote: Thank you for your suggestions! IMO, error cases 3 (hey, two 3s!), 4 and 6 are not so likely, as including any STL container header will make a begin and an end functions declared, though maybe not usabe. In these cases the most probable error case would be 5. > It would be nice for users, but it's certainly more work, and > potentially more places for bugs to creep in. Yeah, you're right. I think I'll leave that for a future patch... Rodrigo
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 3a60d0f..4e13a04 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -1607,6 +1607,8 @@ static tree cp_parser_c_for (cp_parser *, tree, tree); static tree cp_parser_range_for (cp_parser *, tree, tree, tree); +static void cp_parser_perform_range_for_lookup + (tree, tree *, tree *); static tree cp_parser_jump_statement (cp_parser *); static void cp_parser_declaration_statement @@ -8555,14 +8557,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) @@ -8598,44 +8606,41 @@ cp_convert_range_for (tree statement, tree range_decl, tree range_expr) 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); + if (!COMPLETE_TYPE_P (TREE_TYPE (range_temp))) + { + error ("range-based-for expression must be of a complete type"); + begin_expr = end_expr = iter_type = error_mark_node; + } + else + { + /* 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)); + cp_parser_perform_range_for_lookup (range_temp, &begin_expr, &end_expr); + + if (begin_expr == error_mark_node || end_expr == error_mark_node) + /* If one of the expressions is an error do no more checks. */ + begin_expr = end_expr = iter_type = error_mark_node; + else + { + iter_type = cv_unqualified (TREE_TYPE (begin_expr)); + /* 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_expr)))) + error ("inconsistent begin/end types in range-based for: " + "%qT and %qT", + TREE_TYPE (begin_expr), TREE_TYPE (end_expr)); + } } } @@ -8680,6 +8685,84 @@ cp_convert_range_for (tree statement, tree range_decl, tree range_expr) return statement; } +static void +cp_parser_perform_range_for_lookup (tree range, tree *begin, tree *end) +{ + tree id_begin, id_end; + + id_begin = get_identifier ("begin"); + *begin = build_qualified_name (/*type=*/NULL_TREE, + TREE_TYPE (range), + id_begin, + /*template_p=*/false); + *begin = finish_class_member_access_expr(range, *begin, + false, tf_none); + + id_end = get_identifier ("end"); + *end = build_qualified_name (/*type=*/NULL_TREE, + TREE_TYPE (range), + id_end, + /*template_p=*/false); + *end = finish_class_member_access_expr(range, *end, + false, tf_none); + + VEC(tree,gc) *vec; + vec = make_tree_vector (); + + if (*begin != error_mark_node || *end != error_mark_node) + { + if (*begin != error_mark_node) + { + tree instance = TREE_OPERAND (*begin, 0); + tree fn = TREE_OPERAND (*begin, 1); + if (BASELINK_P (fn)) + *begin = build_new_method_call (instance, fn, + NULL, NULL, LOOKUP_NORMAL, + NULL, tf_warning_or_error); + else + *begin = finish_call_expr (*begin, &vec, + /*disallow_virtual=*/false, + /*koenig_p=*/false, + tf_warning_or_error); + } + else + error ("range-based-for expression has an %<end%> member " + "but not a %<begin%>"); + if (*end != error_mark_node) + { + tree instance = TREE_OPERAND (*end, 0); + tree fn = TREE_OPERAND (*end, 1); + if (BASELINK_P (fn)) + *end = build_new_method_call (instance, fn, + NULL, NULL, LOOKUP_NORMAL, + NULL, tf_warning_or_error); + else + *end = finish_call_expr (*end, &vec, + /*disallow_virtual=*/false, + /*koenig_p=*/false, + tf_warning_or_error); + } + else + error ("range-based-for expression has a %<begin%> member " + "but not an %<end%>"); + } + else + { + VEC_safe_push (tree, gc, vec, range); + + *begin = perform_koenig_lookup (id_begin, vec, + /*include_std=*/true); + *begin = finish_call_expr (*begin, &vec, false, true, + tf_warning_or_error); + *end = perform_koenig_lookup (id_end, vec, + /*include_std=*/true); + *end = finish_call_expr (*end, &vec, false, true, + tf_warning_or_error); + + } + release_tree_vector (vec); +} + /* Parse an iteration-statement.