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

login
register
mail settings
Submitter Rodrigo Rivas
Date April 6, 2011, 11:22 p.m.
Message ID <BANLkTiksTqVsCfuH1OL0N3snASCLdr14zw@mail.gmail.com>
Download mbox | patch
Permalink /patch/90104/
State New
Headers show

Comments

Rodrigo Rivas - April 6, 2011, 11:22 p.m.
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 %<for%>" or
"range-based %<for%> 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
Jason Merrill - April 8, 2011, 3:45 p.m.
Looks good, just a couple of tweaks:

On 04/06/2011 07:22 PM, Rodrigo Rivas wrote:
> +         error ("range-based %<for%> expression must have complete type");
> +           error ("range-based %<for%> expression has an %<end%> member "
> +                  "but not a %<begin%>");

Let's give the type of the range initializer in these error messages.

> +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;
> +    }
> +}

Instead of handling this difference here, let's teach finish_call_expr 
to handle a COMPONENT_REF around a BASELINK.

Jason

Patch

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 %<for%> 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 %<for%> expression has an %<end%> member "
+		   "but not a %<begin%>");
+
+	  if (member_end != NULL_TREE)
+	    *end = cp_parser_range_for_member_function (range, id_end);
+	  else
+	    error ("range-based %<for%> expression has a %<begin%> member "
+		   "but not an %<end%>");
+	}
+      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 %<for%> "
+		   "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 %<for%> 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<typename T> 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<typename T> 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<typename T> 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<typename T> 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<typename T> 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<int> (c) != 10)
+    abort();
+  if (test2<int> (a) != 26)
+    abort();
+
+  if (test3<int> (c) != 10)
+    abort();
+  if (test3<int> (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<typename T> int *begin(T &t)
+{
+    T::fail;
+}
+template<typename T> 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'" }
         ;
 }