[Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Mikhail Maltsev]

[-- Attachment #1: Type: text/plain, Size: 113 bytes --]

The attached patch was bootstrapped and regtested on
x86_64-unknown-linux-gnu.

-- 
Regards,
    Mikhail Maltsev

[-- Attachment #2: pr-65882.cl.txt --]
[-- Type: text/plain, Size: 260 bytes --]

gcc/cp/ChangeLog:

2015-04-26  Mikhail Maltsev  <maltsevm@gmail.com>

	* call.c (build_new_op_1): Check tf_warning flag in all cases

gcc/testsuite/ChangeLog:

2015-04-26  Mikhail Maltsev  <maltsevm@gmail.com>

	* g++.dg/diagnostic/inhibit-warn.C: New test.



[-- Attachment #3: pr-65882.patch --]
[-- Type: text/plain, Size: 1927 bytes --]

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 7bdf236..689d542 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5677,8 +5677,9 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
     case TRUTH_ORIF_EXPR:
     case TRUTH_AND_EXPR:
     case TRUTH_OR_EXPR:
-      warn_logical_operator (loc, code, boolean_type_node,
-			     code_orig_arg1, arg1, code_orig_arg2, arg2);
+      if (complain & tf_warning)
+	warn_logical_operator (loc, code, boolean_type_node,
+			       code_orig_arg1, arg1, code_orig_arg2, arg2);
       /* Fall through.  */
     case GT_EXPR:
     case LT_EXPR:
@@ -5686,8 +5687,9 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
     case LE_EXPR:
     case EQ_EXPR:
     case NE_EXPR:
-      if ((code_orig_arg1 == BOOLEAN_TYPE)
-	  ^ (code_orig_arg2 == BOOLEAN_TYPE))
+      if ((complain & tf_warning)
+	  && ((code_orig_arg1 == BOOLEAN_TYPE)
+	      ^ (code_orig_arg2 == BOOLEAN_TYPE)))
 	maybe_warn_bool_compare (loc, code, arg1, arg2);
       /* Fall through.  */
     case PLUS_EXPR:
diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn.C b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn.C
new file mode 100644
index 0000000..5655eb4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn.C
@@ -0,0 +1,32 @@
+// PR c++/65882
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wbool-compare" }
+
+// Check that we don't ICE because of reentering error reporting routines while
+// evaluating template parameters
+
+template<typename>
+struct type_function {
+  static constexpr bool value = false;
+};
+
+template<bool>
+struct dependent_type {
+  typedef int type;
+};
+
+template<typename T>
+typename dependent_type<(5 > type_function<T>::value)>::type
+bar();
+
+template<typename T>
+typename dependent_type<(5 > type_function<T>::value)>::type
+foo()
+{
+  return bar<int>();
+}
+
+int main()
+{
+  foo<int>();
+}

Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Marek Polacek]

This is not my patch, but I'd like to ping it anyway as it also fixes
PR66467 (you might want to add a testcase from this PR as well).

On Sun, Apr 26, 2015 at 02:11:14PM +0300, Mikhail Maltsev wrote:
> The attached patch was bootstrapped and regtested on
> x86_64-unknown-linux-gnu.
> 
> -- 
> Regards,
>     Mikhail Maltsev

> gcc/cp/ChangeLog:
> 
> 2015-04-26  Mikhail Maltsev  <maltsevm@gmail.com>
> 
> 	* call.c (build_new_op_1): Check tf_warning flag in all cases
> 
> gcc/testsuite/ChangeLog:
> 
> 2015-04-26  Mikhail Maltsev  <maltsevm@gmail.com>
> 
> 	* g++.dg/diagnostic/inhibit-warn.C: New test.
> 
> 

> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 7bdf236..689d542 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -5677,8 +5677,9 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
>      case TRUTH_ORIF_EXPR:
>      case TRUTH_AND_EXPR:
>      case TRUTH_OR_EXPR:
> -      warn_logical_operator (loc, code, boolean_type_node,
> -			     code_orig_arg1, arg1, code_orig_arg2, arg2);
> +      if (complain & tf_warning)
> +	warn_logical_operator (loc, code, boolean_type_node,
> +			       code_orig_arg1, arg1, code_orig_arg2, arg2);
>        /* Fall through.  */
>      case GT_EXPR:
>      case LT_EXPR:
> @@ -5686,8 +5687,9 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
>      case LE_EXPR:
>      case EQ_EXPR:
>      case NE_EXPR:
> -      if ((code_orig_arg1 == BOOLEAN_TYPE)
> -	  ^ (code_orig_arg2 == BOOLEAN_TYPE))
> +      if ((complain & tf_warning)
> +	  && ((code_orig_arg1 == BOOLEAN_TYPE)
> +	      ^ (code_orig_arg2 == BOOLEAN_TYPE)))
>  	maybe_warn_bool_compare (loc, code, arg1, arg2);
>        /* Fall through.  */
>      case PLUS_EXPR:
> diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn.C b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn.C
> new file mode 100644
> index 0000000..5655eb4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn.C
> @@ -0,0 +1,32 @@
> +// PR c++/65882
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wbool-compare" }
> +
> +// Check that we don't ICE because of reentering error reporting routines while
> +// evaluating template parameters
> +
> +template<typename>
> +struct type_function {
> +  static constexpr bool value = false;
> +};
> +
> +template<bool>
> +struct dependent_type {
> +  typedef int type;
> +};
> +
> +template<typename T>
> +typename dependent_type<(5 > type_function<T>::value)>::type
> +bar();
> +
> +template<typename T>
> +typename dependent_type<(5 > type_function<T>::value)>::type
> +foo()
> +{
> +  return bar<int>();
> +}
> +
> +int main()
> +{
> +  foo<int>();
> +}


	Marek

Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Jason Merrill]

OK, thanks.

Sorry this took so long to review; please feel free to ping me every week.

Jason

Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Mikhail Maltsev]

[-- Attachment #1: Type: text/plain, Size: 409 bytes --]

On 19.06.2015 19:35, Jason Merrill wrote:
> OK, thanks.
> 
> Sorry this took so long to review; please feel free to ping me every week.
> 
> Jason

I added the testcase from PR66467, bootstrapped and regtested on
x86_64-linux. The final variant is attached. I applied it to trunk.

I see that version 5.2 is set as target milestone for this bug. Should I
backport the patch?

-- 
Regards,
    Mikhail Maltsev

[-- Attachment #2: pr65882-final.patch --]
[-- Type: text/plain, Size: 3433 bytes --]

diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog
index eb5e4c5..6656441 100644
--- a/gcc/cp/ChangeLog
+++ b/gcc/cp/ChangeLog
@@ -1,3 +1,8 @@
+2015-06-20  Mikhail Maltsev  <maltsevm@gmail.com>
+
+	PR c++/65882
+	* call.c (build_new_op_1): Check tf_warning flag in all cases.
+
 2015-06-19  Jason Merrill  <jason@redhat.com>
 
 	PR c++/66585
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 5d1891d..ba5da4c 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5640,8 +5640,9 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
     case TRUTH_ORIF_EXPR:
     case TRUTH_AND_EXPR:
     case TRUTH_OR_EXPR:
-      warn_logical_operator (loc, code, boolean_type_node,
-			     code_orig_arg1, arg1, code_orig_arg2, arg2);
+      if (complain & tf_warning)
+	warn_logical_operator (loc, code, boolean_type_node,
+			       code_orig_arg1, arg1, code_orig_arg2, arg2);
       /* Fall through.  */
     case GT_EXPR:
     case LT_EXPR:
@@ -5649,8 +5650,9 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1,
     case LE_EXPR:
     case EQ_EXPR:
     case NE_EXPR:
-      if ((code_orig_arg1 == BOOLEAN_TYPE)
-	  ^ (code_orig_arg2 == BOOLEAN_TYPE))
+      if ((complain & tf_warning)
+	  && ((code_orig_arg1 == BOOLEAN_TYPE)
+	      ^ (code_orig_arg2 == BOOLEAN_TYPE)))
 	maybe_warn_bool_compare (loc, code, arg1, arg2);
       /* Fall through.  */
     case PLUS_EXPR:
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 42a0ee9d..89b859f 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-06-20  Mikhail Maltsev  <maltsevm@gmail.com>
+
+	PR c++/65882
+	* g++.dg/diagnostic/inhibit-warn-1.C: New test.
+	* g++.dg/diagnostic/inhibit-warn-2.C: New test.
+
 2015-06-19  Eric Botcazou  <ebotcazou@adacore.com>
 
 	* gnat.dg/specs/debug1.ads: Adjust.
diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-1.C b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-1.C
new file mode 100644
index 0000000..5655eb4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-1.C
@@ -0,0 +1,32 @@
+// PR c++/65882
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wbool-compare" }
+
+// Check that we don't ICE because of reentering error reporting routines while
+// evaluating template parameters
+
+template<typename>
+struct type_function {
+  static constexpr bool value = false;
+};
+
+template<bool>
+struct dependent_type {
+  typedef int type;
+};
+
+template<typename T>
+typename dependent_type<(5 > type_function<T>::value)>::type
+bar();
+
+template<typename T>
+typename dependent_type<(5 > type_function<T>::value)>::type
+foo()
+{
+  return bar<int>();
+}
+
+int main()
+{
+  foo<int>();
+}
diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
new file mode 100644
index 0000000..cb16b4c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
@@ -0,0 +1,36 @@
+// PR c++/65882
+// PR c++/66467
+// { dg-do compile }
+
+template <bool>
+struct A
+{
+  typedef int type;
+};
+
+struct B
+{
+  static const int value = 0;
+};
+
+template <class>
+struct C
+{
+  typedef int type;
+};
+
+template <class>
+struct F : B {};
+
+class D
+{
+  template <class Expr>
+  typename A<F<typename C<Expr>::type>::value || B::value>::type
+  operator=(Expr); // { dg-message "declared" }
+};
+
+void fn1()
+{
+  D opt;
+  opt = 0; // { dg-error "private" }
+}

Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Jason Merrill]

On 06/19/2015 08:23 PM, Mikhail Maltsev wrote:
> I see that version 5.2 is set as target milestone for this bug. Should I
> backport the patch?

Please.

Jason

Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Christophe Lyon]

On 22 June 2015 at 18:59, Jason Merrill <jason@redhat.com> wrote:
> On 06/19/2015 08:23 PM, Mikhail Maltsev wrote:
>>
>> I see that version 5.2 is set as target milestone for this bug. Should I
>> backport the patch?
>
>
> Please.
>
> Jason
>

Hi Mikhail,

In the gcc-5-branch, I can see that your new inhibit-warn-2.C test
fails (targets ARM and AArch64).

I can see this error message in g++.log:
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:
In function 'void fn1()':
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3:
error: 'typename A<(F<typename C< <template-parameter-1-1>
>::type>::value || B:: value)>::type D::operator=(Expr) [with Expr =
int; typename A<(F<typename C< <template-parameter-1-1>
>::type>::value || B:: value)>::type = int]' is private
/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7:
error: within this context

Christophe.

Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Mikhail Maltsev]

On 06/24/2015 06:52 PM, Christophe Lyon wrote:
> Hi Mikhail,
> 
> In the gcc-5-branch, I can see that your new inhibit-warn-2.C test
> fails (targets ARM and AArch64).
> 
> I can see this error message in g++.log:
> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:
> In function 'void fn1()':
> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3:
> error: 'typename A<(F<typename C< <template-parameter-1-1>
>> ::type>::value || B:: value)>::type D::operator=(Expr) [with Expr =
> int; typename A<(F<typename C< <template-parameter-1-1>
>> ::type>::value || B:: value)>::type = int]' is private
> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7:
> error: within this context
> 
> Christophe.
> 
Oops. Sorry for that, it seems that I messed up with my testing box and
the backport did not actually get regtested :(.

The problem is caused by difference in wording of diagnostics. GCC 6
gives an error on line 35 and a note on line 29:

$ ./cc1plus ~/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
 void fn1()
/home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7:
error: 'typename A<(F<typename C< <template-parameter-1-1>
>::type>::value || B:: value)>::type D::operator=(Expr) [with Expr =
int; typename A<(F<typename C< <template-parameter-1-1> >::type>::value
|| B:: value)>::type = int]' is private within this context
   opt = 0;
/home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3:
note: declared private here
   operator=(Expr);

GCC 5 gives two errors:

/home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3:
error: 'typename A<(F<typename C< <template-parameter-1-1>
>::type>::value || B:: value)>::type D::operator=(Expr) [with Expr =
int; typename A<(F<typename C< <template-parameter-1-1> >::type>::value
|| B:: value)>::type = int]' is private
   operator=(Expr);
/home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7:
error: within this context
   opt = 0;

It can probably be fixed like this:

diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
index cb16b4c..f658c1d 100644
--- a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
+++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
@@ -26,11 +26,11 @@ class D
 {
   template <class Expr>
   typename A<F<typename C<Expr>::type>::value || B::value>::type
-  operator=(Expr); // { dg-message "declared" }
+  operator=(Expr); // { dg-message "private" }
 };

 void fn1()
 {
   D opt;
-  opt = 0; // { dg-error "private" }
+  opt = 0; // { dg-error "this context" }
 }

But I am not sure, what should I do in this case. Maybe it is better to
remove the failing testcase from GCC 5 branch (provided that
inhibit-warn-1.C tests a fix for the same bug and does not fail)?

-- 
Regards,
    Mikhail Maltsev

Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[James Greenhalgh]

On Wed, Jun 24, 2015 at 09:28:34PM +0100, Mikhail Maltsev wrote:
> On 06/24/2015 06:52 PM, Christophe Lyon wrote:
> > Hi Mikhail,
> > 
> > In the gcc-5-branch, I can see that your new inhibit-warn-2.C test
> > fails (targets ARM and AArch64).
> > 
> > I can see this error message in g++.log:
> > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:
> > In function 'void fn1()':
> > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3:
> > error: 'typename A<(F<typename C< <template-parameter-1-1>
> >> ::type>::value || B:: value)>::type D::operator=(Expr) [with Expr =
> > int; typename A<(F<typename C< <template-parameter-1-1>
> >> ::type>::value || B:: value)>::type = int]' is private
> > /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7:
> > error: within this context
> > 
> > Christophe.
> > 
> Oops. Sorry for that, it seems that I messed up with my testing box and
> the backport did not actually get regtested :(.
> 
> The problem is caused by difference in wording of diagnostics. GCC 6
> gives an error on line 35 and a note on line 29:
> 
> $ ./cc1plus ~/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
>  void fn1()
> /home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7:
> error: 'typename A<(F<typename C< <template-parameter-1-1>
> >::type>::value || B:: value)>::type D::operator=(Expr) [with Expr =
> int; typename A<(F<typename C< <template-parameter-1-1> >::type>::value
> || B:: value)>::type = int]' is private within this context
>    opt = 0;
> /home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3:
> note: declared private here
>    operator=(Expr);
> 
> GCC 5 gives two errors:
> 
> /home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:29:3:
> error: 'typename A<(F<typename C< <template-parameter-1-1>
> >::type>::value || B:: value)>::type D::operator=(Expr) [with Expr =
> int; typename A<(F<typename C< <template-parameter-1-1> >::type>::value
> || B:: value)>::type = int]' is private
>    operator=(Expr);
> /home/miyuki/gcc/src/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C:35:7:
> error: within this context
>    opt = 0;
> 
> It can probably be fixed like this:
> 
> diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
> b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
> index cb16b4c..f658c1d 100644
> --- a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
> +++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
> @@ -26,11 +26,11 @@ class D
>  {
>    template <class Expr>
>    typename A<F<typename C<Expr>::type>::value || B::value>::type
> -  operator=(Expr); // { dg-message "declared" }
> +  operator=(Expr); // { dg-message "private" }
>  };
> 
>  void fn1()
>  {
>    D opt;
> -  opt = 0; // { dg-error "private" }
> +  opt = 0; // { dg-error "this context" }
>  }
> 
> But I am not sure, what should I do in this case. Maybe it is better to
> remove the failing testcase from GCC 5 branch (provided that
> inhibit-warn-1.C tests a fix for the same bug and does not fail)?

Your fix looks sensible to me, and I'd like to see it applied before
the 5.2 cut. Hopefully someone with the authority to OK the fixup will
be along shortly!

Thanks,
James

Re: [Patch, C++, PR65882] Check tf_warning flag in build_new_op_1

[Jason Merrill]

On 06/24/2015 04:28 PM, Mikhail Maltsev wrote:
> It can probably be fixed like this:
>
> diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
> b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
> index cb16b4c..f658c1d 100644
> --- a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
> +++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-2.C
> @@ -26,11 +26,11 @@ class D
>   {
>     template <class Expr>
>     typename A<F<typename C<Expr>::type>::value || B::value>::type
> -  operator=(Expr); // { dg-message "declared" }
> +  operator=(Expr); // { dg-message "private" }
>   };
>
>   void fn1()
>   {
>     D opt;
> -  opt = 0; // { dg-error "private" }
> +  opt = 0; // { dg-error "this context" }
>   }
>
> But I am not sure, what should I do in this case. Maybe it is better to
> remove the failing testcase from GCC 5 branch (provided that
> inhibit-warn-1.C tests a fix for the same bug and does not fail)?

This patch is OK (and, I think, obvious).  I think it's better to keep 
the testcase.

Jason