Patchwork [C++0x] Range-based for statements and ADL

login
register
mail settings
Submitter Rodrigo Rivas
Date March 29, 2011, 12:49 a.m.
Message ID <AANLkTikoD0hoj36RqO163uXedvtxF1ntoageDv_Dzz3M@mail.gmail.com>
Download mbox | patch
Permalink /patch/88706/
State New
Headers show

Comments

Rodrigo Rivas - March 29, 2011, 12:49 a.m.
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).

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...

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
Jonathan Wakely - March 29, 2011, 8:12 a.m.
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
>
Rodrigo Rivas - March 29, 2011, 9:38 a.m.
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'.
Gabriel Dos Reis - March 29, 2011, 3:46 p.m.
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"?
Rodrigo Rivas - March 29, 2011, 4:41 p.m.
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
Jonathan Wakely - March 29, 2011, 6:22 p.m.
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
Rodrigo Rivas - March 29, 2011, 8:33 p.m.
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
Jonathan Wakely - March 29, 2011, 9:13 p.m.
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.
Rodrigo Rivas - March 29, 2011, 10:16 p.m.
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

Patch

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.