diff mbox series

c++: Prevent warnings for value-dependent exprs [PR96742]

Message ID 20201024225231.2369564-1-polacek@redhat.com
State New
Headers show
Series c++: Prevent warnings for value-dependent exprs [PR96742] | expand

Commit Message

Marek Polacek Oct. 24, 2020, 10:52 p.m. UTC
Here, in r11-155, I changed the call to uses_template_parms to
type_dependent_expression_p_push to avoid a crash in C++98 in
value_dependent_expression_p on a non-constant expression.  But that
prompted a host of complaints that we now warn for value-dependent
expressions in templates.  Those warnings are technically valid, but
people still don't want them because they're awkward to avoid.  So let's
partially revert my earlier fix and make sure that we don't ICE in
value_dependent_expression_p by checking potential_constant_expression
first.

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

gcc/cp/ChangeLog:

	PR c++/96675
	PR c++/96742
	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
	type_dependent_expression_p_push.  Only call uses_template_parms
	for expressions that are potential_constant_expression.

gcc/testsuite/ChangeLog:

	PR c++/96675
	PR c++/96742
	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
	* g++.dg/warn/Wtautological-compare3.C: New test.
	* g++.dg/warn/Wtype-limits5.C: New test.
	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
---
 gcc/cp/pt.c                                        |  6 ++++--
 gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
 gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
 gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
 5 files changed, 30 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
 create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C


base-commit: 1aeb7d7d67d167297ca2f4a97ef20f68e7546b4c

Comments

Jason Merrill Oct. 27, 2020, 5:36 p.m. UTC | #1
On 10/24/20 6:52 PM, Marek Polacek wrote:
> Here, in r11-155, I changed the call to uses_template_parms to
> type_dependent_expression_p_push to avoid a crash in C++98 in
> value_dependent_expression_p on a non-constant expression.  But that
> prompted a host of complaints that we now warn for value-dependent
> expressions in templates.  Those warnings are technically valid, but
> people still don't want them because they're awkward to avoid.  So let's
> partially revert my earlier fix and make sure that we don't ICE in
> value_dependent_expression_p by checking potential_constant_expression
> first.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/96675
> 	PR c++/96742
> 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
> 	type_dependent_expression_p_push.  Only call uses_template_parms
> 	for expressions that are potential_constant_expression.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/96675
> 	PR c++/96742
> 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
> 	* g++.dg/warn/Wtautological-compare3.C: New test.
> 	* g++.dg/warn/Wtype-limits5.C: New test.
> 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
> ---
>   gcc/cp/pt.c                                        |  6 ++++--
>   gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
>   gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
>   gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
>   gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
>   5 files changed, 30 insertions(+), 5 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> 
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index dc664ec3798..8aa0bc2c0d8 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -19618,8 +19618,10 @@ tsubst_copy_and_build (tree t,
>         {
>   	/* If T was type-dependent, suppress warnings that depend on the range
>   	   of the types involved.  */
> -	bool was_dep = type_dependent_expression_p_push (t);
> -
> +	++processing_template_decl;
> +	const bool was_dep = (!potential_constant_expression (t)
> +			      || uses_template_parms (t));

We don't want to suppress warnings for a non-constant expression that 
uses no template parms.  So maybe

potential_c_e ? value_d : type_d

?  Or perhaps instantiation_dependent_expression_p.

> +	--processing_template_decl;
>   	tree op0 = RECUR (TREE_OPERAND (t, 0));
>   	tree op1 = RECUR (TREE_OPERAND (t, 1));
>   
> diff --git a/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C b/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
> index 424eb0c3d49..01f691f2878 100644
> --- a/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
> +++ b/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
> @@ -5,8 +5,10 @@ foo (T t, int i)
>   {
>     int m1 = 10 / t;
>     int m2 = 10 / i;
> -  int m3 = 10 / (sizeof(T) - sizeof(int)); // { dg-warning "division by" }
> -  int m4 = 10 / N; // { dg-warning "division by" }
> +  // People don't want to see warnings for type- or value-dependent
> +  // expressions.
> +  int m3 = 10 / (sizeof(T) - sizeof(int)); // { dg-bogus "division by" }
> +  int m4 = 10 / N; // { dg-bogus "division by" }
>     return m1 + m2 + m3 + m4;
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C b/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
> new file mode 100644
> index 00000000000..89bf1b619a6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
> @@ -0,0 +1,11 @@
> +// PR c++/96675
> +// { dg-do compile { target c++11 } }
> +// { dg-additional-options "-Wtautological-compare" }
> +
> +template<char c>
> +constexpr bool f(char d) {
> +    return 'a' <= c && c <= 'z' ? (d | 0x20) == c :
> +        'A' <= c && c <= 'Z' ? (d & ~0x20) == c :
> +        d == c;
> +}
> +static_assert(f<'p'>('P'), "");
> diff --git a/gcc/testsuite/g++.dg/warn/Wtype-limits5.C b/gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> new file mode 100644
> index 00000000000..5e79123b622
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> @@ -0,0 +1,11 @@
> +// PR c++/96742
> +// { dg-additional-options "-Wtype-limits" }
> +
> +template <unsigned N>
> +bool f(unsigned x) {
> +    return unsigned(x < N);
> +}
> +
> +int main() {
> +    f<0>(1);
> +}
> diff --git a/gcc/testsuite/g++.old-deja/g++.pt/crash10.C b/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
> index 012e3d0c11b..a84b19004ee 100644
> --- a/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
> +++ b/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
> @@ -6,7 +6,6 @@ public:
>     enum { val = (N == 0) ? M : GCD<N, M % N>::val };
>   // { dg-error "constant expression" "valid" { target *-*-* } .-1 }
>   // { dg-message "template argument" "valid" { target *-*-* } .-2 }
> -// { dg-warning "division by" "" { target *-*-* } .-3 }
>   };
>   
>   int main() {
> 
> base-commit: 1aeb7d7d67d167297ca2f4a97ef20f68e7546b4c
>
Marek Polacek Oct. 28, 2020, 6 p.m. UTC | #2
On Tue, Oct 27, 2020 at 01:36:30PM -0400, Jason Merrill wrote:
> On 10/24/20 6:52 PM, Marek Polacek wrote:
> > Here, in r11-155, I changed the call to uses_template_parms to
> > type_dependent_expression_p_push to avoid a crash in C++98 in
> > value_dependent_expression_p on a non-constant expression.  But that
> > prompted a host of complaints that we now warn for value-dependent
> > expressions in templates.  Those warnings are technically valid, but
> > people still don't want them because they're awkward to avoid.  So let's
> > partially revert my earlier fix and make sure that we don't ICE in
> > value_dependent_expression_p by checking potential_constant_expression
> > first.
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	PR c++/96675
> > 	PR c++/96742
> > 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
> > 	type_dependent_expression_p_push.  Only call uses_template_parms
> > 	for expressions that are potential_constant_expression.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	PR c++/96675
> > 	PR c++/96742
> > 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
> > 	* g++.dg/warn/Wtautological-compare3.C: New test.
> > 	* g++.dg/warn/Wtype-limits5.C: New test.
> > 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
> > ---
> >   gcc/cp/pt.c                                        |  6 ++++--
> >   gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
> >   gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
> >   gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
> >   gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
> >   5 files changed, 30 insertions(+), 5 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
> >   create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> > 
> > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index dc664ec3798..8aa0bc2c0d8 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -19618,8 +19618,10 @@ tsubst_copy_and_build (tree t,
> >         {
> >   	/* If T was type-dependent, suppress warnings that depend on the range
> >   	   of the types involved.  */
> > -	bool was_dep = type_dependent_expression_p_push (t);
> > -
> > +	++processing_template_decl;
> > +	const bool was_dep = (!potential_constant_expression (t)
> > +			      || uses_template_parms (t));
> 
> We don't want to suppress warnings for a non-constant expression that uses
> no template parms.  So maybe

Fair enough.

> potential_c_e ? value_d : type_d

That works for all the cases I have.

> ?  Or perhaps instantiation_dependent_expression_p.

i_d_e_p would still crash in C++98 :(.

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

-- >8 --
Here, in r11-155, I changed the call to uses_template_parms to
type_dependent_expression_p_push to avoid a crash in C++98 in
value_dependent_expression_p on a non-constant expression.  But that
prompted a host of complaints that we now warn for value-dependent
expressions in templates.  Those warnings are technically valid, but
people still don't want them because they're awkward to avoid.  This
patch uses value_dependent_expression_p or type_dependent_expression_p.
But make sure that we don't ICE in value_dependent_expression_p by
checking potential_constant_expression first.

gcc/cp/ChangeLog:

	PR c++/96675
	PR c++/96742
	* pt.c (tsubst_copy_and_build): Call value_dependent_expression_p or
	type_dependent_expression_p instead of type_dependent_expression_p_push.
	But only call value_dependent_expression_p for expressions that are
	potential_constant_expression.

gcc/testsuite/ChangeLog:

	PR c++/96675
	PR c++/96742
	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
	* g++.dg/warn/Wtautological-compare3.C: New test.
	* g++.dg/warn/Wtype-limits5.C: New test.
	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
---
 gcc/cp/pt.c                                        |  7 +++++--
 gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
 gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
 gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
 5 files changed, 31 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
 create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 3c0f2546489..57db476645e 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -19618,8 +19618,11 @@ tsubst_copy_and_build (tree t,
       {
 	/* If T was type-dependent, suppress warnings that depend on the range
 	   of the types involved.  */
-	bool was_dep = type_dependent_expression_p_push (t);
-
+	++processing_template_decl;
+	const bool was_dep = (potential_constant_expression (t)
+			      ? value_dependent_expression_p (t)
+			      : type_dependent_expression_p (t));
+	--processing_template_decl;
 	tree op0 = RECUR (TREE_OPERAND (t, 0));
 	tree op1 = RECUR (TREE_OPERAND (t, 1));
 
diff --git a/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C b/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
index 424eb0c3d49..01f691f2878 100644
--- a/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
+++ b/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
@@ -5,8 +5,10 @@ foo (T t, int i)
 {
   int m1 = 10 / t;
   int m2 = 10 / i;
-  int m3 = 10 / (sizeof(T) - sizeof(int)); // { dg-warning "division by" }
-  int m4 = 10 / N; // { dg-warning "division by" }
+  // People don't want to see warnings for type- or value-dependent
+  // expressions.
+  int m3 = 10 / (sizeof(T) - sizeof(int)); // { dg-bogus "division by" }
+  int m4 = 10 / N; // { dg-bogus "division by" }
   return m1 + m2 + m3 + m4;
 }
 
diff --git a/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C b/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
new file mode 100644
index 00000000000..89bf1b619a6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
@@ -0,0 +1,11 @@
+// PR c++/96675
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-Wtautological-compare" }
+
+template<char c>
+constexpr bool f(char d) {
+    return 'a' <= c && c <= 'z' ? (d | 0x20) == c :
+        'A' <= c && c <= 'Z' ? (d & ~0x20) == c :
+        d == c;
+}
+static_assert(f<'p'>('P'), "");
diff --git a/gcc/testsuite/g++.dg/warn/Wtype-limits5.C b/gcc/testsuite/g++.dg/warn/Wtype-limits5.C
new file mode 100644
index 00000000000..5e79123b622
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wtype-limits5.C
@@ -0,0 +1,11 @@
+// PR c++/96742
+// { dg-additional-options "-Wtype-limits" }
+
+template <unsigned N>
+bool f(unsigned x) {
+    return unsigned(x < N);
+}
+
+int main() {
+    f<0>(1);
+}
diff --git a/gcc/testsuite/g++.old-deja/g++.pt/crash10.C b/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
index 012e3d0c11b..a84b19004ee 100644
--- a/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
+++ b/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
@@ -6,7 +6,6 @@ public:
   enum { val = (N == 0) ? M : GCD<N, M % N>::val };
 // { dg-error "constant expression" "valid" { target *-*-* } .-1 }
 // { dg-message "template argument" "valid" { target *-*-* } .-2 }
-// { dg-warning "division by" "" { target *-*-* } .-3 }
 };
 
 int main() {

base-commit: 176b8b9679dfec881b7cf379f808cca3950b1e74
Jason Merrill Oct. 28, 2020, 6:46 p.m. UTC | #3
On 10/28/20 2:00 PM, Marek Polacek wrote:
> On Tue, Oct 27, 2020 at 01:36:30PM -0400, Jason Merrill wrote:
>> On 10/24/20 6:52 PM, Marek Polacek wrote:
>>> Here, in r11-155, I changed the call to uses_template_parms to
>>> type_dependent_expression_p_push to avoid a crash in C++98 in
>>> value_dependent_expression_p on a non-constant expression.  But that
>>> prompted a host of complaints that we now warn for value-dependent
>>> expressions in templates.  Those warnings are technically valid, but
>>> people still don't want them because they're awkward to avoid.  So let's
>>> partially revert my earlier fix and make sure that we don't ICE in
>>> value_dependent_expression_p by checking potential_constant_expression
>>> first.
>>>
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	PR c++/96675
>>> 	PR c++/96742
>>> 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
>>> 	type_dependent_expression_p_push.  Only call uses_template_parms
>>> 	for expressions that are potential_constant_expression.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	PR c++/96675
>>> 	PR c++/96742
>>> 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
>>> 	* g++.dg/warn/Wtautological-compare3.C: New test.
>>> 	* g++.dg/warn/Wtype-limits5.C: New test.
>>> 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
>>> ---
>>>    gcc/cp/pt.c                                        |  6 ++++--
>>>    gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
>>>    gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
>>>    gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
>>>    gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
>>>    5 files changed, 30 insertions(+), 5 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
>>>    create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C
>>>
>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
>>> index dc664ec3798..8aa0bc2c0d8 100644
>>> --- a/gcc/cp/pt.c
>>> +++ b/gcc/cp/pt.c
>>> @@ -19618,8 +19618,10 @@ tsubst_copy_and_build (tree t,
>>>          {
>>>    	/* If T was type-dependent, suppress warnings that depend on the range
>>>    	   of the types involved.  */
>>> -	bool was_dep = type_dependent_expression_p_push (t);
>>> -
>>> +	++processing_template_decl;
>>> +	const bool was_dep = (!potential_constant_expression (t)
>>> +			      || uses_template_parms (t));
>>
>> We don't want to suppress warnings for a non-constant expression that uses
>> no template parms.  So maybe
> 
> Fair enough.
> 
>> potential_c_e ? value_d : type_d
> 
> That works for all the cases I have.
> 
>> ?  Or perhaps instantiation_dependent_expression_p.
> 
> i_d_e_p would still crash in C++98 :(.

Perhaps we should protect the value_d call in i_d_e_p with potential_c_e?

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?

OK.

> -- >8 --
> Here, in r11-155, I changed the call to uses_template_parms to
> type_dependent_expression_p_push to avoid a crash in C++98 in
> value_dependent_expression_p on a non-constant expression.  But that
> prompted a host of complaints that we now warn for value-dependent
> expressions in templates.  Those warnings are technically valid, but
> people still don't want them because they're awkward to avoid.  This
> patch uses value_dependent_expression_p or type_dependent_expression_p.
> But make sure that we don't ICE in value_dependent_expression_p by
> checking potential_constant_expression first.
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/96675
> 	PR c++/96742
> 	* pt.c (tsubst_copy_and_build): Call value_dependent_expression_p or
> 	type_dependent_expression_p instead of type_dependent_expression_p_push.
> 	But only call value_dependent_expression_p for expressions that are
> 	potential_constant_expression.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/96675
> 	PR c++/96742
> 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
> 	* g++.dg/warn/Wtautological-compare3.C: New test.
> 	* g++.dg/warn/Wtype-limits5.C: New test.
> 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
> ---
>   gcc/cp/pt.c                                        |  7 +++++--
>   gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
>   gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
>   gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
>   gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
>   5 files changed, 31 insertions(+), 5 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> 
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 3c0f2546489..57db476645e 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -19618,8 +19618,11 @@ tsubst_copy_and_build (tree t,
>         {
>   	/* If T was type-dependent, suppress warnings that depend on the range
>   	   of the types involved.  */
> -	bool was_dep = type_dependent_expression_p_push (t);
> -
> +	++processing_template_decl;
> +	const bool was_dep = (potential_constant_expression (t)
> +			      ? value_dependent_expression_p (t)
> +			      : type_dependent_expression_p (t));
> +	--processing_template_decl;
>   	tree op0 = RECUR (TREE_OPERAND (t, 0));
>   	tree op1 = RECUR (TREE_OPERAND (t, 1));
>   
> diff --git a/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C b/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
> index 424eb0c3d49..01f691f2878 100644
> --- a/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
> +++ b/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
> @@ -5,8 +5,10 @@ foo (T t, int i)
>   {
>     int m1 = 10 / t;
>     int m2 = 10 / i;
> -  int m3 = 10 / (sizeof(T) - sizeof(int)); // { dg-warning "division by" }
> -  int m4 = 10 / N; // { dg-warning "division by" }
> +  // People don't want to see warnings for type- or value-dependent
> +  // expressions.
> +  int m3 = 10 / (sizeof(T) - sizeof(int)); // { dg-bogus "division by" }
> +  int m4 = 10 / N; // { dg-bogus "division by" }
>     return m1 + m2 + m3 + m4;
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C b/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
> new file mode 100644
> index 00000000000..89bf1b619a6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
> @@ -0,0 +1,11 @@
> +// PR c++/96675
> +// { dg-do compile { target c++11 } }
> +// { dg-additional-options "-Wtautological-compare" }
> +
> +template<char c>
> +constexpr bool f(char d) {
> +    return 'a' <= c && c <= 'z' ? (d | 0x20) == c :
> +        'A' <= c && c <= 'Z' ? (d & ~0x20) == c :
> +        d == c;
> +}
> +static_assert(f<'p'>('P'), "");
> diff --git a/gcc/testsuite/g++.dg/warn/Wtype-limits5.C b/gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> new file mode 100644
> index 00000000000..5e79123b622
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> @@ -0,0 +1,11 @@
> +// PR c++/96742
> +// { dg-additional-options "-Wtype-limits" }
> +
> +template <unsigned N>
> +bool f(unsigned x) {
> +    return unsigned(x < N);
> +}
> +
> +int main() {
> +    f<0>(1);
> +}
> diff --git a/gcc/testsuite/g++.old-deja/g++.pt/crash10.C b/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
> index 012e3d0c11b..a84b19004ee 100644
> --- a/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
> +++ b/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
> @@ -6,7 +6,6 @@ public:
>     enum { val = (N == 0) ? M : GCD<N, M % N>::val };
>   // { dg-error "constant expression" "valid" { target *-*-* } .-1 }
>   // { dg-message "template argument" "valid" { target *-*-* } .-2 }
> -// { dg-warning "division by" "" { target *-*-* } .-3 }
>   };
>   
>   int main() {
> 
> base-commit: 176b8b9679dfec881b7cf379f808cca3950b1e74
>
Marek Polacek Oct. 28, 2020, 9:29 p.m. UTC | #4
On Wed, Oct 28, 2020 at 02:46:36PM -0400, Jason Merrill wrote:
> On 10/28/20 2:00 PM, Marek Polacek wrote:
> > On Tue, Oct 27, 2020 at 01:36:30PM -0400, Jason Merrill wrote:
> > > On 10/24/20 6:52 PM, Marek Polacek wrote:
> > > > Here, in r11-155, I changed the call to uses_template_parms to
> > > > type_dependent_expression_p_push to avoid a crash in C++98 in
> > > > value_dependent_expression_p on a non-constant expression.  But that
> > > > prompted a host of complaints that we now warn for value-dependent
> > > > expressions in templates.  Those warnings are technically valid, but
> > > > people still don't want them because they're awkward to avoid.  So let's
> > > > partially revert my earlier fix and make sure that we don't ICE in
> > > > value_dependent_expression_p by checking potential_constant_expression
> > > > first.
> > > > 
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > > 	PR c++/96675
> > > > 	PR c++/96742
> > > > 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
> > > > 	type_dependent_expression_p_push.  Only call uses_template_parms
> > > > 	for expressions that are potential_constant_expression.
> > > > 
> > > > gcc/testsuite/ChangeLog:
> > > > 
> > > > 	PR c++/96675
> > > > 	PR c++/96742
> > > > 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
> > > > 	* g++.dg/warn/Wtautological-compare3.C: New test.
> > > > 	* g++.dg/warn/Wtype-limits5.C: New test.
> > > > 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
> > > > ---
> > > >    gcc/cp/pt.c                                        |  6 ++++--
> > > >    gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
> > > >    gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
> > > >    gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
> > > >    gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
> > > >    5 files changed, 30 insertions(+), 5 deletions(-)
> > > >    create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
> > > >    create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> > > > 
> > > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > > > index dc664ec3798..8aa0bc2c0d8 100644
> > > > --- a/gcc/cp/pt.c
> > > > +++ b/gcc/cp/pt.c
> > > > @@ -19618,8 +19618,10 @@ tsubst_copy_and_build (tree t,
> > > >          {
> > > >    	/* If T was type-dependent, suppress warnings that depend on the range
> > > >    	   of the types involved.  */
> > > > -	bool was_dep = type_dependent_expression_p_push (t);
> > > > -
> > > > +	++processing_template_decl;
> > > > +	const bool was_dep = (!potential_constant_expression (t)
> > > > +			      || uses_template_parms (t));
> > > 
> > > We don't want to suppress warnings for a non-constant expression that uses
> > > no template parms.  So maybe
> > 
> > Fair enough.
> > 
> > > potential_c_e ? value_d : type_d
> > 
> > That works for all the cases I have.
> > 
> > > ?  Or perhaps instantiation_dependent_expression_p.
> > 
> > i_d_e_p would still crash in C++98 :(.
> 
> Perhaps we should protect the value_d call in i_d_e_p with potential_c_e?

Yeah, probably.  But then we should also guard the call to value_d in
uses_template_parms.  I can apply such a patch if it tests fine, if you
want.

> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> 
> OK.

Thanks,

Marek
Jason Merrill Oct. 28, 2020, 9:48 p.m. UTC | #5
On 10/28/20 5:29 PM, Marek Polacek wrote:
> On Wed, Oct 28, 2020 at 02:46:36PM -0400, Jason Merrill wrote:
>> On 10/28/20 2:00 PM, Marek Polacek wrote:
>>> On Tue, Oct 27, 2020 at 01:36:30PM -0400, Jason Merrill wrote:
>>>> On 10/24/20 6:52 PM, Marek Polacek wrote:
>>>>> Here, in r11-155, I changed the call to uses_template_parms to
>>>>> type_dependent_expression_p_push to avoid a crash in C++98 in
>>>>> value_dependent_expression_p on a non-constant expression.  But that
>>>>> prompted a host of complaints that we now warn for value-dependent
>>>>> expressions in templates.  Those warnings are technically valid, but
>>>>> people still don't want them because they're awkward to avoid.  So let's
>>>>> partially revert my earlier fix and make sure that we don't ICE in
>>>>> value_dependent_expression_p by checking potential_constant_expression
>>>>> first.
>>>>>
>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>> 	PR c++/96675
>>>>> 	PR c++/96742
>>>>> 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
>>>>> 	type_dependent_expression_p_push.  Only call uses_template_parms
>>>>> 	for expressions that are potential_constant_expression.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>> 	PR c++/96675
>>>>> 	PR c++/96742
>>>>> 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
>>>>> 	* g++.dg/warn/Wtautological-compare3.C: New test.
>>>>> 	* g++.dg/warn/Wtype-limits5.C: New test.
>>>>> 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
>>>>> ---
>>>>>     gcc/cp/pt.c                                        |  6 ++++--
>>>>>     gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
>>>>>     gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
>>>>>     gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
>>>>>     gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
>>>>>     5 files changed, 30 insertions(+), 5 deletions(-)
>>>>>     create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
>>>>>     create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C
>>>>>
>>>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
>>>>> index dc664ec3798..8aa0bc2c0d8 100644
>>>>> --- a/gcc/cp/pt.c
>>>>> +++ b/gcc/cp/pt.c
>>>>> @@ -19618,8 +19618,10 @@ tsubst_copy_and_build (tree t,
>>>>>           {
>>>>>     	/* If T was type-dependent, suppress warnings that depend on the range
>>>>>     	   of the types involved.  */
>>>>> -	bool was_dep = type_dependent_expression_p_push (t);
>>>>> -
>>>>> +	++processing_template_decl;
>>>>> +	const bool was_dep = (!potential_constant_expression (t)
>>>>> +			      || uses_template_parms (t));
>>>>
>>>> We don't want to suppress warnings for a non-constant expression that uses
>>>> no template parms.  So maybe
>>>
>>> Fair enough.
>>>
>>>> potential_c_e ? value_d : type_d
>>>
>>> That works for all the cases I have.
>>>
>>>> ?  Or perhaps instantiation_dependent_expression_p.
>>>
>>> i_d_e_p would still crash in C++98 :(.
>>
>> Perhaps we should protect the value_d call in i_d_e_p with potential_c_e?
> 
> Yeah, probably.  But then we should also guard the call to value_d in
> uses_template_parms.  I can apply such a patch if it tests fine, if you
> want.

Or change uses_template_parms to use i_d.

>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
>>
>> OK.
> 
> Thanks,
> 
> Marek
>
Marek Polacek Oct. 29, 2020, 2:45 a.m. UTC | #6
On Wed, Oct 28, 2020 at 05:48:08PM -0400, Jason Merrill wrote:
> On 10/28/20 5:29 PM, Marek Polacek wrote:
> > On Wed, Oct 28, 2020 at 02:46:36PM -0400, Jason Merrill wrote:
> > > On 10/28/20 2:00 PM, Marek Polacek wrote:
> > > > On Tue, Oct 27, 2020 at 01:36:30PM -0400, Jason Merrill wrote:
> > > > > On 10/24/20 6:52 PM, Marek Polacek wrote:
> > > > > > Here, in r11-155, I changed the call to uses_template_parms to
> > > > > > type_dependent_expression_p_push to avoid a crash in C++98 in
> > > > > > value_dependent_expression_p on a non-constant expression.  But that
> > > > > > prompted a host of complaints that we now warn for value-dependent
> > > > > > expressions in templates.  Those warnings are technically valid, but
> > > > > > people still don't want them because they're awkward to avoid.  So let's
> > > > > > partially revert my earlier fix and make sure that we don't ICE in
> > > > > > value_dependent_expression_p by checking potential_constant_expression
> > > > > > first.
> > > > > > 
> > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> > > > > > 
> > > > > > gcc/cp/ChangeLog:
> > > > > > 
> > > > > > 	PR c++/96675
> > > > > > 	PR c++/96742
> > > > > > 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
> > > > > > 	type_dependent_expression_p_push.  Only call uses_template_parms
> > > > > > 	for expressions that are potential_constant_expression.
> > > > > > 
> > > > > > gcc/testsuite/ChangeLog:
> > > > > > 
> > > > > > 	PR c++/96675
> > > > > > 	PR c++/96742
> > > > > > 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
> > > > > > 	* g++.dg/warn/Wtautological-compare3.C: New test.
> > > > > > 	* g++.dg/warn/Wtype-limits5.C: New test.
> > > > > > 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
> > > > > > ---
> > > > > >     gcc/cp/pt.c                                        |  6 ++++--
> > > > > >     gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
> > > > > >     gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
> > > > > >     gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
> > > > > >     gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
> > > > > >     5 files changed, 30 insertions(+), 5 deletions(-)
> > > > > >     create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
> > > > > >     create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C
> > > > > > 
> > > > > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > > > > > index dc664ec3798..8aa0bc2c0d8 100644
> > > > > > --- a/gcc/cp/pt.c
> > > > > > +++ b/gcc/cp/pt.c
> > > > > > @@ -19618,8 +19618,10 @@ tsubst_copy_and_build (tree t,
> > > > > >           {
> > > > > >     	/* If T was type-dependent, suppress warnings that depend on the range
> > > > > >     	   of the types involved.  */
> > > > > > -	bool was_dep = type_dependent_expression_p_push (t);
> > > > > > -
> > > > > > +	++processing_template_decl;
> > > > > > +	const bool was_dep = (!potential_constant_expression (t)
> > > > > > +			      || uses_template_parms (t));
> > > > > 
> > > > > We don't want to suppress warnings for a non-constant expression that uses
> > > > > no template parms.  So maybe
> > > > 
> > > > Fair enough.
> > > > 
> > > > > potential_c_e ? value_d : type_d
> > > > 
> > > > That works for all the cases I have.
> > > > 
> > > > > ?  Or perhaps instantiation_dependent_expression_p.
> > > > 
> > > > i_d_e_p would still crash in C++98 :(.
> > > 
> > > Perhaps we should protect the value_d call in i_d_e_p with potential_c_e?
> > 
> > Yeah, probably.  But then we should also guard the call to value_d in
> > uses_template_parms.  I can apply such a patch if it tests fine, if you
> > want.
> 
> Or change uses_template_parms to use i_d.

Experimenting with this revealed a curious issue: when we have
__PRETTY_FUNCTION__ in a template function, we set its DECL_VALUE_EXPR
to error_mark_node (cp_make_fname_decl), so potential_c_e returns false
when it gets it, but value_dependent_expression_p handles it specially
and says true.  So this patch

--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -27277,7 +27277,8 @@ bool
 instantiation_dependent_expression_p (tree expression)
 {
   return (instantiation_dependent_uneval_expression_p (expression)
-     || value_dependent_expression_p (expression));
+     || (potential_constant_expression (expression)
+         && value_dependent_expression_p (expression)));
 }
 
 /* Like type_dependent_expression_p, but it also works while not processing

breaks lambda-generic-pretty1.C.  ISTM that potential_c_e should return
true for a DECL_PRETTY_FUNCTION_P with error DECL_VALUE_EXPR.

Marek
Jason Merrill Oct. 29, 2020, 3:15 p.m. UTC | #7
On 10/28/20 10:45 PM, Marek Polacek wrote:
> On Wed, Oct 28, 2020 at 05:48:08PM -0400, Jason Merrill wrote:
>> On 10/28/20 5:29 PM, Marek Polacek wrote:
>>> On Wed, Oct 28, 2020 at 02:46:36PM -0400, Jason Merrill wrote:
>>>> On 10/28/20 2:00 PM, Marek Polacek wrote:
>>>>> On Tue, Oct 27, 2020 at 01:36:30PM -0400, Jason Merrill wrote:
>>>>>> On 10/24/20 6:52 PM, Marek Polacek wrote:
>>>>>>> Here, in r11-155, I changed the call to uses_template_parms to
>>>>>>> type_dependent_expression_p_push to avoid a crash in C++98 in
>>>>>>> value_dependent_expression_p on a non-constant expression.  But that
>>>>>>> prompted a host of complaints that we now warn for value-dependent
>>>>>>> expressions in templates.  Those warnings are technically valid, but
>>>>>>> people still don't want them because they're awkward to avoid.  So let's
>>>>>>> partially revert my earlier fix and make sure that we don't ICE in
>>>>>>> value_dependent_expression_p by checking potential_constant_expression
>>>>>>> first.
>>>>>>>
>>>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
>>>>>>>
>>>>>>> gcc/cp/ChangeLog:
>>>>>>>
>>>>>>> 	PR c++/96675
>>>>>>> 	PR c++/96742
>>>>>>> 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
>>>>>>> 	type_dependent_expression_p_push.  Only call uses_template_parms
>>>>>>> 	for expressions that are potential_constant_expression.
>>>>>>>
>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>
>>>>>>> 	PR c++/96675
>>>>>>> 	PR c++/96742
>>>>>>> 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
>>>>>>> 	* g++.dg/warn/Wtautological-compare3.C: New test.
>>>>>>> 	* g++.dg/warn/Wtype-limits5.C: New test.
>>>>>>> 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
>>>>>>> ---
>>>>>>>      gcc/cp/pt.c                                        |  6 ++++--
>>>>>>>      gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C         |  6 ++++--
>>>>>>>      gcc/testsuite/g++.dg/warn/Wtautological-compare3.C | 11 +++++++++++
>>>>>>>      gcc/testsuite/g++.dg/warn/Wtype-limits5.C          | 11 +++++++++++
>>>>>>>      gcc/testsuite/g++.old-deja/g++.pt/crash10.C        |  1 -
>>>>>>>      5 files changed, 30 insertions(+), 5 deletions(-)
>>>>>>>      create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
>>>>>>>      create mode 100644 gcc/testsuite/g++.dg/warn/Wtype-limits5.C
>>>>>>>
>>>>>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
>>>>>>> index dc664ec3798..8aa0bc2c0d8 100644
>>>>>>> --- a/gcc/cp/pt.c
>>>>>>> +++ b/gcc/cp/pt.c
>>>>>>> @@ -19618,8 +19618,10 @@ tsubst_copy_and_build (tree t,
>>>>>>>            {
>>>>>>>      	/* If T was type-dependent, suppress warnings that depend on the range
>>>>>>>      	   of the types involved.  */
>>>>>>> -	bool was_dep = type_dependent_expression_p_push (t);
>>>>>>> -
>>>>>>> +	++processing_template_decl;
>>>>>>> +	const bool was_dep = (!potential_constant_expression (t)
>>>>>>> +			      || uses_template_parms (t));
>>>>>>
>>>>>> We don't want to suppress warnings for a non-constant expression that uses
>>>>>> no template parms.  So maybe
>>>>>
>>>>> Fair enough.
>>>>>
>>>>>> potential_c_e ? value_d : type_d
>>>>>
>>>>> That works for all the cases I have.
>>>>>
>>>>>> ?  Or perhaps instantiation_dependent_expression_p.
>>>>>
>>>>> i_d_e_p would still crash in C++98 :(.
>>>>
>>>> Perhaps we should protect the value_d call in i_d_e_p with potential_c_e?
>>>
>>> Yeah, probably.  But then we should also guard the call to value_d in
>>> uses_template_parms.  I can apply such a patch if it tests fine, if you
>>> want.
>>
>> Or change uses_template_parms to use i_d.
> 
> Experimenting with this revealed a curious issue: when we have
> __PRETTY_FUNCTION__ in a template function, we set its DECL_VALUE_EXPR
> to error_mark_node (cp_make_fname_decl), so potential_c_e returns false
> when it gets it, but value_dependent_expression_p handles it specially
> and says true.  So this patch
> 
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -27277,7 +27277,8 @@ bool
>   instantiation_dependent_expression_p (tree expression)
>   {
>     return (instantiation_dependent_uneval_expression_p (expression)
> -     || value_dependent_expression_p (expression));
> +     || (potential_constant_expression (expression)
> +         && value_dependent_expression_p (expression)));
>   }
>   
>   /* Like type_dependent_expression_p, but it also works while not processing
> 
> breaks lambda-generic-pretty1.C.  ISTM that potential_c_e should return
> true for a DECL_PRETTY_FUNCTION_P with error DECL_VALUE_EXPR.

Agreed.

Jason
Iain Sandoe Oct. 31, 2020, 10:09 a.m. UTC | #8
Hi Marek,

Marek Polacek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> On Wed, Oct 28, 2020 at 02:46:36PM -0400, Jason Merrill wrote:
>> On 10/28/20 2:00 PM, Marek Polacek wrote:
>>> On Tue, Oct 27, 2020 at 01:36:30PM -0400, Jason Merrill wrote:
>>>> On 10/24/20 6:52 PM, Marek Polacek wrote:
>>>>> Here, in r11-155, I changed the call to uses_template_parms to
>>>>> type_dependent_expression_p_push to avoid a crash in C++98 in
>>>>> value_dependent_expression_p on a non-constant expression.  But that
>>>>> prompted a host of complaints that we now warn for value-dependent
>>>>> expressions in templates.  Those warnings are technically valid, but
>>>>> people still don't want them because they're awkward to avoid.  So let's
>>>>> partially revert my earlier fix and make sure that we don't ICE in
>>>>> value_dependent_expression_p by checking potential_constant_expression
>>>>> first.
>>>>> 
>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
>>>>> 
>>>>> gcc/cp/ChangeLog:
>>>>> 
>>>>> 	PR c++/96675
>>>>> 	PR c++/96742
>>>>> 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
>>>>> 	type_dependent_expression_p_push.  Only call uses_template_parms
>>>>> 	for expressions that are potential_constant_expression.
>>>>> 
>>>>> gcc/testsuite/ChangeLog:
>>>>> 
>>>>> 	PR c++/96675
>>>>> 	PR c++/96742
>>>>> 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
>>>>> 	* g++.dg/warn/Wtautological-compare3.C: New test.
>>>>> 	* g++.dg/warn/Wtype-limits5.C: New test.
>>>>> 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
>>>>> —
> 
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
>> 
>> OK.
> 
> Thanks,

This regresses Objective-C++ template-4.mm with an ICE (because we don’t have a
check for message_send_expr, which can’t be constexpr).

I am testing the following fix (which is mostly Objective-C-local), but open to alternate
solutions.  Personally, I prefer to avoid double-negatives in code but because of the 
fallthrough to the sorry case, it seems hard to avoid.

OK for master if testing is sucessful?
(I assume we will also need to do the same on 10.x if you backported already)

cheers
Iain

========

[PATCH] Objective-C++ : Fix ICE in potential_constant_expression_1.

We cannot, as things stand, handle Objective-C tree codes in
the switch and deal with this by calling out to a function that
has a dummy version when Objective-C is not enabled.

Because of the way the logic works (with a fall through to a
'sorry' in case of unhandled expressions), the function reports
cases that are known to be unsuitable for constant exprs. The
dummy function always reports 'false' and thus will fall through
to the 'sorry'.

gcc/c-family/ChangeLog:

	* c-objc.h (objc_non_constant_expr_p): New.
	* stub-objc.c (objc_non_constant_expr_p): New.

gcc/cp/ChangeLog:

	* constexpr.c (potential_constant_expression_1): Handle
	expressions known to be non-constant for Objective-C.

gcc/objc/ChangeLog:

	* objc-act.c (objc_non_constant_expr_p): New.
---
 gcc/c-family/c-objc.h    |  1 +
 gcc/c-family/stub-objc.c |  6 ++++++
 gcc/cp/constexpr.c       |  2 +-
 gcc/objc/objc-act.c      | 16 +++++++++++++++-
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/gcc/c-family/c-objc.h b/gcc/c-family/c-objc.h
index 6025283e5cf..4577e4f1c7f 100644
--- a/gcc/c-family/c-objc.h
+++ b/gcc/c-family/c-objc.h
@@ -101,6 +101,7 @@ extern void objc_add_synthesize_declaration (location_t, tree);
 extern void objc_add_dynamic_declaration (location_t, tree);
 extern const char * objc_maybe_printable_name (tree, int);
 extern bool objc_is_property_ref (tree);
+extern bool objc_non_constant_expr_p (tree);
 extern bool objc_string_ref_type_p (tree);
 extern void objc_check_format_arg (tree, tree);
 extern void objc_finish_function (void);
diff --git a/gcc/c-family/stub-objc.c b/gcc/c-family/stub-objc.c
index c30f0b3c67d..9ced68b2b3e 100644
--- a/gcc/c-family/stub-objc.c
+++ b/gcc/c-family/stub-objc.c
@@ -331,6 +331,12 @@ objc_is_property_ref (tree ARG_UNUSED (node))
   return 0;
 }
 
+bool
+objc_non_constant_expr_p (tree ARG_UNUSED (node))
+{
+  return 0;
+}
+
 tree
 objc_maybe_build_component_ref (tree ARG_UNUSED (datum), tree ARG_UNUSED (component))
 {
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index b46824f128d..03a826ba250 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -8410,7 +8410,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
       return false;
 
     default:
-      if (objc_is_property_ref (t))
+      if (objc_non_constant_expr_p (t))
 	return false;
 
       sorry ("unexpected AST of kind %s", get_tree_code_name (TREE_CODE (t)));
diff --git a/gcc/objc/objc-act.c b/gcc/objc/objc-act.c
index 0393bc44500..c0d07ae9182 100644
--- a/gcc/objc/objc-act.c
+++ b/gcc/objc/objc-act.c
@@ -1720,7 +1720,6 @@ objc_build_class_component_ref (tree class_name, tree property_ident)
 }
 
 
-
 /* This is used because we don't want to expose PROPERTY_REF to the
    C/C++ frontends.  Maybe we should!  */
 bool
@@ -1732,6 +1731,21 @@ objc_is_property_ref (tree node)
     return false;
 }
 
+/* We use this to report tree codes that are known to be invalid in const-
+   expression contexts.  */
+bool
+objc_non_constant_expr_p (tree node)
+{
+  switch (TREE_CODE (node))
+    {
+      default:
+	return false;
+      case MESSAGE_SEND_EXPR:
+      case PROPERTY_REF:
+	return true;
+    }
+}
+
 /* This function builds a setter call for a PROPERTY_REF (real, for a
    declared property, or artificial, for a dot-syntax accessor which
    is not corresponding to a property).  'lhs' must be a PROPERTY_REF
Marek Polacek Nov. 3, 2020, 6:40 p.m. UTC | #9
On Sat, Oct 31, 2020 at 10:09:32AM +0000, Iain Sandoe wrote:
> Hi Marek,
> 
> Marek Polacek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> > On Wed, Oct 28, 2020 at 02:46:36PM -0400, Jason Merrill wrote:
> >> On 10/28/20 2:00 PM, Marek Polacek wrote:
> >>> On Tue, Oct 27, 2020 at 01:36:30PM -0400, Jason Merrill wrote:
> >>>> On 10/24/20 6:52 PM, Marek Polacek wrote:
> >>>>> Here, in r11-155, I changed the call to uses_template_parms to
> >>>>> type_dependent_expression_p_push to avoid a crash in C++98 in
> >>>>> value_dependent_expression_p on a non-constant expression.  But that
> >>>>> prompted a host of complaints that we now warn for value-dependent
> >>>>> expressions in templates.  Those warnings are technically valid, but
> >>>>> people still don't want them because they're awkward to avoid.  So let's
> >>>>> partially revert my earlier fix and make sure that we don't ICE in
> >>>>> value_dependent_expression_p by checking potential_constant_expression
> >>>>> first.
> >>>>> 
> >>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> >>>>> 
> >>>>> gcc/cp/ChangeLog:
> >>>>> 
> >>>>> 	PR c++/96675
> >>>>> 	PR c++/96742
> >>>>> 	* pt.c (tsubst_copy_and_build): Call uses_template_parms instead of
> >>>>> 	type_dependent_expression_p_push.  Only call uses_template_parms
> >>>>> 	for expressions that are potential_constant_expression.
> >>>>> 
> >>>>> gcc/testsuite/ChangeLog:
> >>>>> 
> >>>>> 	PR c++/96675
> >>>>> 	PR c++/96742
> >>>>> 	* g++.dg/warn/Wdiv-by-zero-3.C: Turn dg-warning into dg-bogus.
> >>>>> 	* g++.dg/warn/Wtautological-compare3.C: New test.
> >>>>> 	* g++.dg/warn/Wtype-limits5.C: New test.
> >>>>> 	* g++.old-deja/g++.pt/crash10.C: Remove dg-warning.
> >>>>> —
> > 
> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> >> 
> >> OK.
> > 
> > Thanks,
> 
> This regresses Objective-C++ template-4.mm with an ICE (because we don’t have a
> check for message_send_expr, which can’t be constexpr).

Sorry for failing to test Objective-C++ properly.

> I am testing the following fix (which is mostly Objective-C-local), but open to alternate
> solutions.  Personally, I prefer to avoid double-negatives in code but because of the 
> fallthrough to the sorry case, it seems hard to avoid.
> 
> OK for master if testing is sucessful?
> (I assume we will also need to do the same on 10.x if you backported already)
> 
> cheers
> Iain
> 
> ========
> 
> [PATCH] Objective-C++ : Fix ICE in potential_constant_expression_1.
> 
> We cannot, as things stand, handle Objective-C tree codes in
> the switch and deal with this by calling out to a function that
> has a dummy version when Objective-C is not enabled.
> 
> Because of the way the logic works (with a fall through to a
> 'sorry' in case of unhandled expressions), the function reports
> cases that are known to be unsuitable for constant exprs. The
> dummy function always reports 'false' and thus will fall through
> to the 'sorry'.
> 
> gcc/c-family/ChangeLog:
> 
> 	* c-objc.h (objc_non_constant_expr_p): New.
> 	* stub-objc.c (objc_non_constant_expr_p): New.
> 
> gcc/cp/ChangeLog:
> 
> 	* constexpr.c (potential_constant_expression_1): Handle
> 	expressions known to be non-constant for Objective-C.
> 
> gcc/objc/ChangeLog:
> 
> 	* objc-act.c (objc_non_constant_expr_p): New.
> ---
>  gcc/c-family/c-objc.h    |  1 +
>  gcc/c-family/stub-objc.c |  6 ++++++
>  gcc/cp/constexpr.c       |  2 +-
>  gcc/objc/objc-act.c      | 16 +++++++++++++++-
>  4 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/c-family/c-objc.h b/gcc/c-family/c-objc.h
> index 6025283e5cf..4577e4f1c7f 100644
> --- a/gcc/c-family/c-objc.h
> +++ b/gcc/c-family/c-objc.h
> @@ -101,6 +101,7 @@ extern void objc_add_synthesize_declaration (location_t, tree);
>  extern void objc_add_dynamic_declaration (location_t, tree);
>  extern const char * objc_maybe_printable_name (tree, int);
>  extern bool objc_is_property_ref (tree);
> +extern bool objc_non_constant_expr_p (tree);
>  extern bool objc_string_ref_type_p (tree);
>  extern void objc_check_format_arg (tree, tree);
>  extern void objc_finish_function (void);
> diff --git a/gcc/c-family/stub-objc.c b/gcc/c-family/stub-objc.c
> index c30f0b3c67d..9ced68b2b3e 100644
> --- a/gcc/c-family/stub-objc.c
> +++ b/gcc/c-family/stub-objc.c
> @@ -331,6 +331,12 @@ objc_is_property_ref (tree ARG_UNUSED (node))
>    return 0;
>  }
>  
> +bool
> +objc_non_constant_expr_p (tree ARG_UNUSED (node))

In new code we can just omit the name of the parameter: (tree)

I have no other comments otherwise.  I think you can go ahead with this
change.  Thanks.

Marek
diff mbox series

Patch

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index dc664ec3798..8aa0bc2c0d8 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -19618,8 +19618,10 @@  tsubst_copy_and_build (tree t,
       {
 	/* If T was type-dependent, suppress warnings that depend on the range
 	   of the types involved.  */
-	bool was_dep = type_dependent_expression_p_push (t);
-
+	++processing_template_decl;
+	const bool was_dep = (!potential_constant_expression (t)
+			      || uses_template_parms (t));
+	--processing_template_decl;
 	tree op0 = RECUR (TREE_OPERAND (t, 0));
 	tree op1 = RECUR (TREE_OPERAND (t, 1));
 
diff --git a/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C b/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
index 424eb0c3d49..01f691f2878 100644
--- a/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
+++ b/gcc/testsuite/g++.dg/warn/Wdiv-by-zero-3.C
@@ -5,8 +5,10 @@  foo (T t, int i)
 {
   int m1 = 10 / t;
   int m2 = 10 / i;
-  int m3 = 10 / (sizeof(T) - sizeof(int)); // { dg-warning "division by" }
-  int m4 = 10 / N; // { dg-warning "division by" }
+  // People don't want to see warnings for type- or value-dependent
+  // expressions.
+  int m3 = 10 / (sizeof(T) - sizeof(int)); // { dg-bogus "division by" }
+  int m4 = 10 / N; // { dg-bogus "division by" }
   return m1 + m2 + m3 + m4;
 }
 
diff --git a/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C b/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
new file mode 100644
index 00000000000..89bf1b619a6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wtautological-compare3.C
@@ -0,0 +1,11 @@ 
+// PR c++/96675
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-Wtautological-compare" }
+
+template<char c>
+constexpr bool f(char d) {
+    return 'a' <= c && c <= 'z' ? (d | 0x20) == c :
+        'A' <= c && c <= 'Z' ? (d & ~0x20) == c :
+        d == c;
+}
+static_assert(f<'p'>('P'), "");
diff --git a/gcc/testsuite/g++.dg/warn/Wtype-limits5.C b/gcc/testsuite/g++.dg/warn/Wtype-limits5.C
new file mode 100644
index 00000000000..5e79123b622
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wtype-limits5.C
@@ -0,0 +1,11 @@ 
+// PR c++/96742
+// { dg-additional-options "-Wtype-limits" }
+
+template <unsigned N>
+bool f(unsigned x) {
+    return unsigned(x < N);
+}
+
+int main() {
+    f<0>(1);
+}
diff --git a/gcc/testsuite/g++.old-deja/g++.pt/crash10.C b/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
index 012e3d0c11b..a84b19004ee 100644
--- a/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
+++ b/gcc/testsuite/g++.old-deja/g++.pt/crash10.C
@@ -6,7 +6,6 @@  public:
   enum { val = (N == 0) ? M : GCD<N, M % N>::val };
 // { dg-error "constant expression" "valid" { target *-*-* } .-1 }
 // { dg-message "template argument" "valid" { target *-*-* } .-2 }
-// { dg-warning "division by" "" { target *-*-* } .-3 }
 };
 
 int main() {