diff mbox series

C++ PATCH for c++/87594, constexpr rejects-valid with range-based for

Message ID 20181012005601.GK19003@redhat.com
State New
Headers show
Series C++ PATCH for c++/87594, constexpr rejects-valid with range-based for | expand

Commit Message

Marek Polacek Oct. 12, 2018, 12:56 a.m. UTC 
Here potential_constant_expression_1 rejects the testcase because the body of
the for loop calls a non-constexpr function.  But the range is empty so the
function would never get called.
The trick with evaluating the for-condition doesn't work here, because we're
dealing with a converted range-based for and can't evaluate

  __for_begin != __for_end

because the constexpr cache doesn't have the values of these two VAR_DECLs.

So either we can use this ugly hack (more specialized), or just not check the
body of the loop (more general), similarly to what we do (don't do) with
switch.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-10-11  Marek Polacek  <polacek@redhat.com>

	PR c++/87594 - constexpr rejects-valid with range-based for.
	* constexpr.c (potential_constant_expression_1) <case FOR_STMT>: Return
	true for a converted ange-based for-statement.

	* g++.dg/cpp1y/constexpr-loop8.C: New test.

Comments

Jason Merrill Oct. 24, 2018, 9:16 p.m. UTC | #1 
On 10/11/18 8:56 PM, Marek Polacek wrote:
> Here potential_constant_expression_1 rejects the testcase because the body of
> the for loop calls a non-constexpr function.  But the range is empty so the
> function would never get called.
> The trick with evaluating the for-condition doesn't work here, because we're
> dealing with a converted range-based for and can't evaluate
> 
>    __for_begin != __for_end
> 
> because the constexpr cache doesn't have the values of these two VAR_DECLs.


> So either we can use this ugly hack (more specialized), or just not check the
> body of the loop (more general), similarly to what we do (don't do) with
> switch.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2018-10-11  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/87594 - constexpr rejects-valid with range-based for.
> 	* constexpr.c (potential_constant_expression_1) <case FOR_STMT>: Return
> 	true for a converted ange-based for-statement.
> 
> 	* g++.dg/cpp1y/constexpr-loop8.C: New test.
> 
> diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
> index 4fa8c965a9d..685ca743859 100644
> --- gcc/cp/constexpr.c
> +++ gcc/cp/constexpr.c
> @@ -5827,6 +5827,17 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>   	    tmp = cxx_eval_outermost_constant_expr (tmp, true);
>   	  if (integer_zerop (tmp))
>   	    return true;
> +	  /* See if this is
> +	     __for_begin != __for_end
> +	     cp_convert_range_for created for us.  If so, this is a converted
> +	     range-based for-statement, and we're not able to evaluate this
> +	     condition, so we might end up skipping the body entirely.  */
> +	  else if (TREE_CODE (tmp) == NE_EXPR
> +		   && VAR_P (TREE_OPERAND (tmp, 0))
> +		   && DECL_NAME (TREE_OPERAND (tmp, 0)) == for_begin_identifier
> +		   && VAR_P (TREE_OPERAND (tmp, 1))
> +		   && DECL_NAME (TREE_OPERAND (tmp, 1)) == for_end_identifier)
> +	    return true;

This seems over-specific; any other condition we can't immediately 
evaluate also might not ever be true.  So I think we want to return here 
unless the condition is always true.  Likewise for WHILE_STMT.

Jason
Marek Polacek Oct. 29, 2018, 6:07 p.m. UTC | #2 
On Wed, Oct 24, 2018 at 05:16:33PM -0400, Jason Merrill wrote:
> On 10/11/18 8:56 PM, Marek Polacek wrote:
> > Here potential_constant_expression_1 rejects the testcase because the body of
> > the for loop calls a non-constexpr function.  But the range is empty so the
> > function would never get called.
> > The trick with evaluating the for-condition doesn't work here, because we're
> > dealing with a converted range-based for and can't evaluate
> > 
> >    __for_begin != __for_end
> > 
> > because the constexpr cache doesn't have the values of these two VAR_DECLs.
> 
> 
> > So either we can use this ugly hack (more specialized), or just not check the
> > body of the loop (more general), similarly to what we do (don't do) with
> > switch.
> > 
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> > 
> > 2018-10-11  Marek Polacek  <polacek@redhat.com>
> > 
> > 	PR c++/87594 - constexpr rejects-valid with range-based for.
> > 	* constexpr.c (potential_constant_expression_1) <case FOR_STMT>: Return
> > 	true for a converted ange-based for-statement.
> > 
> > 	* g++.dg/cpp1y/constexpr-loop8.C: New test.
> > 
> > diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
> > index 4fa8c965a9d..685ca743859 100644
> > --- gcc/cp/constexpr.c
> > +++ gcc/cp/constexpr.c
> > @@ -5827,6 +5827,17 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
> >   	    tmp = cxx_eval_outermost_constant_expr (tmp, true);
> >   	  if (integer_zerop (tmp))
> >   	    return true;
> > +	  /* See if this is
> > +	     __for_begin != __for_end
> > +	     cp_convert_range_for created for us.  If so, this is a converted
> > +	     range-based for-statement, and we're not able to evaluate this
> > +	     condition, so we might end up skipping the body entirely.  */
> > +	  else if (TREE_CODE (tmp) == NE_EXPR
> > +		   && VAR_P (TREE_OPERAND (tmp, 0))
> > +		   && DECL_NAME (TREE_OPERAND (tmp, 0)) == for_begin_identifier
> > +		   && VAR_P (TREE_OPERAND (tmp, 1))
> > +		   && DECL_NAME (TREE_OPERAND (tmp, 1)) == for_end_identifier)
> > +	    return true;
> 
> This seems over-specific; any other condition we can't immediately evaluate
> also might not ever be true.  So I think we want to return here unless the
> condition is always true.  Likewise for WHILE_STMT.

Yeah, that makes a lot of sense.  Thus:

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-10-29  Marek Polacek  <polacek@redhat.com>

	PR c++/87594 - constexpr rejects-valid with range-based for.
	* constexpr.c (potential_constant_expression_1): If the condition
	can't be evaluated, return true.

	* g++.dg/cpp1y/constexpr-loop8.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 4fa8c965a9d..7692b1727da 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -5825,7 +5825,9 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
 	{
 	  if (!processing_template_decl)
 	    tmp = cxx_eval_outermost_constant_expr (tmp, true);
-	  if (integer_zerop (tmp))
+	  /* If we couldn't evaluate the condition, it might not ever be
+	     true.  */
+	  if (!integer_onep (tmp))
 	    return true;
 	}
       if (!RECUR (FOR_EXPR (t), any))
@@ -5853,7 +5855,8 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
 	return false;
       if (!processing_template_decl)
 	tmp = cxx_eval_outermost_constant_expr (tmp, true);
-      if (integer_zerop (tmp))
+      /* If we couldn't evaluate the condition, it might not ever be true.  */
+      if (!integer_onep (tmp))
 	return true;
       if (!RECUR (WHILE_BODY (t), any))
 	return false;
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-loop8.C gcc/testsuite/g++.dg/cpp1y/constexpr-loop8.C
index e69de29bb2d..bf132f2484e 100644
--- gcc/testsuite/g++.dg/cpp1y/constexpr-loop8.C
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-loop8.C
@@ -0,0 +1,44 @@
+// PR c++/87594
+// { dg-do compile { target c++14 } }
+
+constexpr bool always_false() { return false; }
+int f() { return 1; }
+
+constexpr int
+fn1()
+{
+  struct empty_range {
+    constexpr int* begin() { return 0; }
+    constexpr int* end() { return 0; }
+  } e;
+  for (auto x : e)
+    f();
+  return 0;
+}
+
+constexpr int
+fn2 ()
+{
+  int a[] = { 1, 2, 3 };
+  for (auto x : a)
+    f(); // { dg-error "call to non-.constexpr. function" }
+  return 0;
+}
+
+constexpr int
+fn3 ()
+{
+  __extension__ int a[] = { };
+  for (auto x : a)
+    f();
+  return 0;
+}
+
+
+void
+bar ()
+{
+  constexpr int i1 = fn1 ();
+  constexpr int i2 = fn2 (); // { dg-message "in .constexpr. expansion of " }
+  constexpr int i3 = fn3 ();
+}
Jason Merrill Oct. 29, 2018, 7:34 p.m. UTC | #3 
On 10/29/18 2:07 PM, Marek Polacek wrote:
> On Wed, Oct 24, 2018 at 05:16:33PM -0400, Jason Merrill wrote:
>> On 10/11/18 8:56 PM, Marek Polacek wrote:
>>> Here potential_constant_expression_1 rejects the testcase because the body of
>>> the for loop calls a non-constexpr function.  But the range is empty so the
>>> function would never get called.
>>> The trick with evaluating the for-condition doesn't work here, because we're
>>> dealing with a converted range-based for and can't evaluate
>>>
>>>     __for_begin != __for_end
>>>
>>> because the constexpr cache doesn't have the values of these two VAR_DECLs.
>>
>>
>>> So either we can use this ugly hack (more specialized), or just not check the
>>> body of the loop (more general), similarly to what we do (don't do) with
>>> switch.
>>>
>>> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>>>
>>> 2018-10-11  Marek Polacek  <polacek@redhat.com>
>>>
>>> 	PR c++/87594 - constexpr rejects-valid with range-based for.
>>> 	* constexpr.c (potential_constant_expression_1) <case FOR_STMT>: Return
>>> 	true for a converted ange-based for-statement.
>>>
>>> 	* g++.dg/cpp1y/constexpr-loop8.C: New test.
>>>
>>> diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
>>> index 4fa8c965a9d..685ca743859 100644
>>> --- gcc/cp/constexpr.c
>>> +++ gcc/cp/constexpr.c
>>> @@ -5827,6 +5827,17 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>>>    	    tmp = cxx_eval_outermost_constant_expr (tmp, true);
>>>    	  if (integer_zerop (tmp))
>>>    	    return true;
>>> +	  /* See if this is
>>> +	     __for_begin != __for_end
>>> +	     cp_convert_range_for created for us.  If so, this is a converted
>>> +	     range-based for-statement, and we're not able to evaluate this
>>> +	     condition, so we might end up skipping the body entirely.  */
>>> +	  else if (TREE_CODE (tmp) == NE_EXPR
>>> +		   && VAR_P (TREE_OPERAND (tmp, 0))
>>> +		   && DECL_NAME (TREE_OPERAND (tmp, 0)) == for_begin_identifier
>>> +		   && VAR_P (TREE_OPERAND (tmp, 1))
>>> +		   && DECL_NAME (TREE_OPERAND (tmp, 1)) == for_end_identifier)
>>> +	    return true;
>>
>> This seems over-specific; any other condition we can't immediately evaluate
>> also might not ever be true.  So I think we want to return here unless the
>> condition is always true.  Likewise for WHILE_STMT.
> 
> Yeah, that makes a lot of sense.  Thus:
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2018-10-29  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/87594 - constexpr rejects-valid with range-based for.
> 	* constexpr.c (potential_constant_expression_1): If the condition
> 	can't be evaluated, return true.

OK.

Jason
diff mbox series

Patch

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 4fa8c965a9d..685ca743859 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -5827,6 +5827,17 @@  potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
 	    tmp = cxx_eval_outermost_constant_expr (tmp, true);
 	  if (integer_zerop (tmp))
 	    return true;
+	  /* See if this is
+	     __for_begin != __for_end
+	     cp_convert_range_for created for us.  If so, this is a converted
+	     range-based for-statement, and we're not able to evaluate this
+	     condition, so we might end up skipping the body entirely.  */
+	  else if (TREE_CODE (tmp) == NE_EXPR
+		   && VAR_P (TREE_OPERAND (tmp, 0))
+		   && DECL_NAME (TREE_OPERAND (tmp, 0)) == for_begin_identifier
+		   && VAR_P (TREE_OPERAND (tmp, 1))
+		   && DECL_NAME (TREE_OPERAND (tmp, 1)) == for_end_identifier)
+	    return true;
 	}
       if (!RECUR (FOR_EXPR (t), any))
 	return false;
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-loop8.C gcc/testsuite/g++.dg/cpp1y/constexpr-loop8.C
index e69de29bb2d..bf132f2484e 100644
--- gcc/testsuite/g++.dg/cpp1y/constexpr-loop8.C
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-loop8.C
@@ -0,0 +1,44 @@ 
+// PR c++/87594
+// { dg-do compile { target c++14 } }
+
+constexpr bool always_false() { return false; }
+int f() { return 1; }
+
+constexpr int
+fn1()
+{
+  struct empty_range {
+    constexpr int* begin() { return 0; }
+    constexpr int* end() { return 0; }
+  } e;
+  for (auto x : e)
+    f();
+  return 0;
+}
+
+constexpr int
+fn2 ()
+{
+  int a[] = { 1, 2, 3 };
+  for (auto x : a)
+    f(); // { dg-error "call to non-.constexpr. function" }
+  return 0;
+}
+
+constexpr int
+fn3 ()
+{
+  __extension__ int a[] = { };
+  for (auto x : a)
+    f();
+  return 0;
+}
+
+
+void
+bar ()
+{
+  constexpr int i1 = fn1 ();
+  constexpr int i2 = fn2 (); // { dg-message "in .constexpr. expansion of " }
+  constexpr int i3 = fn3 ();
+}