[PATCH] Fix a few simple cases where -Wparentheses does not warn for omitted middle value

[PATCH] Fix a few simple cases where -Wparentheses does not warn for omitted middle value

[Bernd Edlinger]

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

Hi,

I've noticed that there is already a -Wparentheses warning for code like

  int y = x == 2 ?: 1

=> warning: the omitted middle operand in ?: will always be 'true',
suggest explicit middle operand [-Wparentheses]

But it is not emitted for code that uses bool, like:

void foo(bool x)
{
   int y = x ?: 1;

and under C it is not emitted for compound expressions
that end with a comparison, like:

   int y = (i++,i==1) ?: 1;

C++ is OK, but does only miss to warn on the bool data type.

The attached patch should fix these warnings.


Bootstrap and reg-testing is still running,
Is it OK for trunk after reg-testing?



Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-omitted-cond-op.diff --]
[-- Type: text/x-patch; name="patch-omitted-cond-op.diff", Size: 3518 bytes --]

gcc/c-family:
2016-09-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-common.c (warn_for_omitted_condop): Also warn for boolean data.

gcc/c:
2016-09-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-parser.c (c_parser_conditional_expression): Pass the rightmost
	COMPOUND_EXPR to warn_for_omitted_condop.

testsuite:
2016-09-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/warn-ommitted-condop.c: Add more test cases.


Index: gcc/c/c-parser.c
===================================================================
--- gcc/c/c-parser.c	(revision 240001)
+++ gcc/c/c-parser.c	(working copy)
@@ -6423,16 +6423,20 @@ c_parser_conditional_expression (c_parser *parser,
   if (c_parser_next_token_is (parser, CPP_COLON))
     {
       tree eptype = NULL_TREE;
+      tree e;
 
       middle_loc = c_parser_peek_token (parser)->location;
       pedwarn (middle_loc, OPT_Wpedantic, 
 	       "ISO C forbids omitting the middle term of a ?: expression");
-      warn_for_omitted_condop (middle_loc, cond.value);
       if (TREE_CODE (cond.value) == EXCESS_PRECISION_EXPR)
 	{
 	  eptype = TREE_TYPE (cond.value);
 	  cond.value = TREE_OPERAND (cond.value, 0);
 	}
+      e = cond.value;
+      while (TREE_CODE (e) == COMPOUND_EXPR)
+	e = TREE_OPERAND (e, 1);
+      warn_for_omitted_condop (middle_loc, e);
       /* Make sure first operand is calculated only once.  */
       exp1.value = c_save_expr (default_conversion (cond.value));
       if (eptype)
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 240001)
+++ gcc/c-family/c-common.c	(working copy)
@@ -10619,7 +10619,8 @@ fold_offsetof (tree expr)
 void
 warn_for_omitted_condop (location_t location, tree cond) 
 { 
-  if (truth_value_p (TREE_CODE (cond))) 
+  if (TREE_CODE (TREE_TYPE (cond)) == BOOLEAN_TYPE
+      || truth_value_p (TREE_CODE (cond))) 
       warning_at (location, OPT_Wparentheses, 
 		"the omitted middle operand in ?: will always be %<true%>, "
 		"suggest explicit middle operand");
Index: gcc/testsuite/c-c++-common/warn-ommitted-condop.c
===================================================================
--- gcc/testsuite/c-c++-common/warn-ommitted-condop.c	(revision 240001)
+++ gcc/testsuite/c-c++-common/warn-ommitted-condop.c	(working copy)
@@ -1,8 +1,12 @@
 /* { dg-options "-Wparentheses -ftrack-macro-expansion=0" } */
 
+#ifndef __cplusplus
+#define bool _Bool
+#endif
+
 extern void f2 (int);
 
-void bar (int x, int y, int z)
+void bar (int x, int y, int z, bool b)
 {
 #define T(op) f2 (x op y ? : 1) 
 #define T2(op) f2 (x op y ? 2 : 1) 
@@ -16,6 +20,8 @@ extern void f2 (int);
   T(||); /* { dg-warning "omitted middle operand" } */
   T(&&); /* { dg-warning "omitted middle operand" } */
   f2 (!x ? : 1);  /* { dg-warning "omitted middle operand" } */
+  f2 ((x,!x) ? : 1);  /* { dg-warning "omitted middle operand" } */
+  f2 ((x,y,!x) ? : 1);  /* { dg-warning "omitted middle operand" } */
   T2(<); /* { dg-bogus "omitted middle operand" } */
   T2(>); /* { dg-bogus "omitted middle operand" } */
   T2(==); /* { dg-bogus "omitted middle operand" } */
@@ -26,4 +32,5 @@ extern void f2 (int);
   T(*); /* { dg-bogus "omitted middle operand" } */
   T(/); /* { dg-bogus "omitted middle operand" } */
   T(^); /* { dg-bogus "omitted middle operand" } */
+  f2 (b ? : 1);  /* { dg-warning "omitted middle operand" } */
 }

[PATCHv2] Fix a few simple cases where -Wparentheses does not warn for omitted middle value

[Bernd Edlinger]

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

On 09/05/16 23:50, Bernd Edlinger wrote:
> Hi,
>
> I've noticed that there is already a -Wparentheses warning for code like
>
>   int y = x == 2 ?: 1
>
> => warning: the omitted middle operand in ?: will always be 'true',
> suggest explicit middle operand [-Wparentheses]
>
> But it is not emitted for code that uses bool, like:
>
> void foo(bool x)
> {
>    int y = x ?: 1;
>
> and under C it is not emitted for compound expressions
> that end with a comparison, like:
>
>    int y = (i++,i==1) ?: 1;
>
> C++ is OK, but does only miss to warn on the bool data type.
>
> The attached patch should fix these warnings.
>

Well, reg-testing showed few test cases were broken, that made me
aware of an issue with templates when the LHS of ?: is dependent.

In that case the type is not available at the template declaration,
and the warning cannot be generated at the declaration but only when
the template is instantiated.  The new patch fixes this, and a
pre-existing issue, entered as PR 77496, when the type can not be
implicitly converted to boolean.


Boot-strapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-omitted-cond-op.diff --]
[-- Type: text/x-patch; name="patch-omitted-cond-op.diff", Size: 7288 bytes --]

gcc/c-family:
2016-09-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c++/77496
	* c-common.c (warn_for_omitted_condop): Also warn for boolean data.

gcc/c:
2016-09-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c++/77496
	* c-parser.c (c_parser_conditional_expression): Pass the rightmost
	COMPOUND_EXPR to warn_for_omitted_condop.

gcc/cp:
2016-09-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c++/77496
	* call.c (build_conditional_expr_1): Call warn_for_omitted_condop.
	* class.c (instantiate_type): Look through the SAVE_EXPR.

gcc/testsuite:
2016-09-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c++/77496
	* c-c++-common/warn-ommitted-condop.c: Add more test cases.
	* g++.dg/ext/pr77496.C: New test.
	* g++.dg/warn/pr77496.C: New test.


Index: gcc/c/c-parser.c
===================================================================
--- gcc/c/c-parser.c	(revision 240001)
+++ gcc/c/c-parser.c	(working copy)
@@ -6423,16 +6423,20 @@ c_parser_conditional_expression (c_parser *parser,
   if (c_parser_next_token_is (parser, CPP_COLON))
     {
       tree eptype = NULL_TREE;
+      tree e;
 
       middle_loc = c_parser_peek_token (parser)->location;
-      pedwarn (middle_loc, OPT_Wpedantic, 
+      pedwarn (middle_loc, OPT_Wpedantic,
 	       "ISO C forbids omitting the middle term of a ?: expression");
-      warn_for_omitted_condop (middle_loc, cond.value);
       if (TREE_CODE (cond.value) == EXCESS_PRECISION_EXPR)
 	{
 	  eptype = TREE_TYPE (cond.value);
 	  cond.value = TREE_OPERAND (cond.value, 0);
 	}
+      e = cond.value;
+      while (TREE_CODE (e) == COMPOUND_EXPR)
+	e = TREE_OPERAND (e, 1);
+      warn_for_omitted_condop (middle_loc, e);
       /* Make sure first operand is calculated only once.  */
       exp1.value = c_save_expr (default_conversion (cond.value));
       if (eptype)
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 240001)
+++ gcc/c-family/c-common.c	(working copy)
@@ -10613,17 +10613,21 @@ fold_offsetof (tree expr)
   return convert (size_type_node, fold_offsetof_1 (expr));
 }
 
-/* Warn for A ?: C expressions (with B omitted) where A is a boolean 
+/* Warn for A ?: C expressions (with B omitted) where A is a boolean
    expression, because B will always be true. */
 
 void
-warn_for_omitted_condop (location_t location, tree cond) 
-{ 
-  if (truth_value_p (TREE_CODE (cond))) 
-      warning_at (location, OPT_Wparentheses, 
+warn_for_omitted_condop (location_t location, tree cond)
+{
+  /* In C++ template declarations it can happen that the type is dependent
+     and not yet known, thus TREE_TYPE (cond) == NULL_TREE.  */
+  if (truth_value_p (TREE_CODE (cond))
+      || (TREE_TYPE (cond) != NULL_TREE &&
+	  TREE_CODE (TREE_TYPE (cond)) == BOOLEAN_TYPE))
+      warning_at (location, OPT_Wparentheses,
 		"the omitted middle operand in ?: will always be %<true%>, "
 		"suggest explicit middle operand");
-} 
+}
 
 /* Give an error for storing into ARG, which is 'const'.  USE indicates
    how ARG was being used.  */
Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c	(revision 240001)
+++ gcc/cp/call.c	(working copy)
@@ -4653,9 +4653,12 @@ build_conditional_expr_1 (location_t loc, tree arg
   if (!arg2)
     {
       if (complain & tf_error)
-	pedwarn (loc, OPT_Wpedantic, 
+	pedwarn (loc, OPT_Wpedantic,
 		 "ISO C++ forbids omitting the middle term of a ?: expression");
 
+      if ((complain & tf_warning) && !truth_value_p (TREE_CODE (arg1)))
+	warn_for_omitted_condop (loc, arg1);
+
       /* Make sure that lvalues remain lvalues.  See g++.oliva/ext1.C.  */
       if (lvalue_p (arg1))
 	arg2 = arg1 = cp_stabilize_reference (arg1);
Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c	(revision 240001)
+++ gcc/cp/class.c	(working copy)
@@ -8262,7 +8262,12 @@ instantiate_type (tree lhstype, tree rhs, tsubst_f
       return error_mark_node;
     }
 
-  /* There only a few kinds of expressions that may have a type
+  /* If we instantiate a template, and it is a A ?: C expression
+     with omitted B, look through the SAVE_EXPR.  */
+  if (TREE_CODE (rhs) == SAVE_EXPR)
+    rhs = TREE_OPERAND (rhs, 0);
+
+  /* There are only a few kinds of expressions that may have a type
      dependent on overload resolution.  */
   gcc_assert (TREE_CODE (rhs) == ADDR_EXPR
 	      || TREE_CODE (rhs) == COMPONENT_REF
Index: gcc/testsuite/c-c++-common/warn-ommitted-condop.c
===================================================================
--- gcc/testsuite/c-c++-common/warn-ommitted-condop.c	(revision 240001)
+++ gcc/testsuite/c-c++-common/warn-ommitted-condop.c	(working copy)
@@ -1,11 +1,15 @@
 /* { dg-options "-Wparentheses -ftrack-macro-expansion=0" } */
 
+#ifndef __cplusplus
+#define bool _Bool
+#endif
+
 extern void f2 (int);
 
-void bar (int x, int y, int z)
+void bar (int x, int y, int z, bool b)
 {
-#define T(op) f2 (x op y ? : 1) 
-#define T2(op) f2 (x op y ? 2 : 1) 
+#define T(op) f2 (x op y ? : 1)
+#define T2(op) f2 (x op y ? 2 : 1)
 
   T(<); /* { dg-warning "omitted middle operand" } */
   T(>); /* { dg-warning "omitted middle operand" } */
@@ -16,6 +20,8 @@ extern void f2 (int);
   T(||); /* { dg-warning "omitted middle operand" } */
   T(&&); /* { dg-warning "omitted middle operand" } */
   f2 (!x ? : 1);  /* { dg-warning "omitted middle operand" } */
+  f2 ((x,!x) ? : 1);  /* { dg-warning "omitted middle operand" } */
+  f2 ((x,y,!x) ? : 1);  /* { dg-warning "omitted middle operand" } */
   T2(<); /* { dg-bogus "omitted middle operand" } */
   T2(>); /* { dg-bogus "omitted middle operand" } */
   T2(==); /* { dg-bogus "omitted middle operand" } */
@@ -26,4 +32,5 @@ extern void f2 (int);
   T(*); /* { dg-bogus "omitted middle operand" } */
   T(/); /* { dg-bogus "omitted middle operand" } */
   T(^); /* { dg-bogus "omitted middle operand" } */
+  f2 (b ? : 1);  /* { dg-warning "omitted middle operand" } */
 }
Index: gcc/testsuite/g++.dg/ext/pr77496.C
===================================================================
--- gcc/testsuite/g++.dg/ext/pr77496.C	(revision 0)
+++ gcc/testsuite/g++.dg/ext/pr77496.C	(working copy)
@@ -0,0 +1,21 @@
+// { dg-do compile }
+// { dg-options "" }
+
+template <class x>
+class z : x
+{
+public:
+  bool zz () { return false; }
+  int f () { return zz ? : 1; } // { dg-error "cannot convert" }
+};
+
+class t
+{
+};
+
+int
+main ()
+{
+  z<t> x;
+  return x.f ();
+}
Index: gcc/testsuite/g++.dg/warn/pr77496.C
===================================================================
--- gcc/testsuite/g++.dg/warn/pr77496.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/pr77496.C	(working copy)
@@ -0,0 +1,21 @@
+// { dg-do compile }
+// { dg-options "-Wparentheses" }
+
+template <class x>
+class z : x
+{
+public:
+  bool zz () { return false; }
+  int f () { return zz () ? : 1; } // { dg-warning "omitted middle operand" }
+};
+
+class t
+{
+};
+
+int
+main ()
+{
+  z<t> x;
+  return x.f ();
+}

Re: [PATCHv2] Fix a few simple cases where -Wparentheses does not warn for omitted middle value

[Jeff Law]

On 09/06/2016 04:07 PM, Bernd Edlinger wrote:
> On 09/05/16 23:50, Bernd Edlinger wrote:
>> > Hi,
>> >
>> > I've noticed that there is already a -Wparentheses warning for code like
>> >
>> >   int y = x == 2 ?: 1
>> >
>> > => warning: the omitted middle operand in ?: will always be 'true',
>> > suggest explicit middle operand [-Wparentheses]
>> >
>> > But it is not emitted for code that uses bool, like:
>> >
>> > void foo(bool x)
>> > {
>> >    int y = x ?: 1;
>> >
>> > and under C it is not emitted for compound expressions
>> > that end with a comparison, like:
>> >
>> >    int y = (i++,i==1) ?: 1;
>> >
>> > C++ is OK, but does only miss to warn on the bool data type.
>> >
>> > The attached patch should fix these warnings.
>> >
> Well, reg-testing showed few test cases were broken, that made me
> aware of an issue with templates when the LHS of ?: is dependent.
>
> In that case the type is not available at the template declaration,
> and the warning cannot be generated at the declaration but only when
> the template is instantiated.  The new patch fixes this, and a
> pre-existing issue, entered as PR 77496, when the type can not be
> implicitly converted to boolean.
>
>
> Boot-strapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
>
>
> patch-omitted-cond-op.diff
>
>
> gcc/c-family:
> 2016-09-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77496
> 	* c-common.c (warn_for_omitted_condop): Also warn for boolean data.
>
> gcc/c:
> 2016-09-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77496
> 	* c-parser.c (c_parser_conditional_expression): Pass the rightmost
> 	COMPOUND_EXPR to warn_for_omitted_condop.
>
> gcc/cp:
> 2016-09-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77496
> 	* call.c (build_conditional_expr_1): Call warn_for_omitted_condop.
> 	* class.c (instantiate_type): Look through the SAVE_EXPR.
>
> gcc/testsuite:
> 2016-09-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77496
> 	* c-c++-common/warn-ommitted-condop.c: Add more test cases.
> 	* g++.dg/ext/pr77496.C: New test.
> 	* g++.dg/warn/pr77496.C: New test.
>
>
> Index: gcc/c/c-parser.c
> ===================================================================
> --- gcc/c/c-parser.c	(revision 240001)
> +++ gcc/c/c-parser.c	(working copy)
> @@ -6423,16 +6423,20 @@ c_parser_conditional_expression (c_parser *parser,
>    if (c_parser_next_token_is (parser, CPP_COLON))
>      {
>        tree eptype = NULL_TREE;
> +      tree e;
Move the declaration to where "e" is initialized.

> Index: gcc/c-family/c-common.c
> ===================================================================
> --- gcc/c-family/c-common.c	(revision 240001)
> +++ gcc/c-family/c-common.c	(working copy)
> @@ -10613,17 +10613,21 @@ fold_offsetof (tree expr)
>    return convert (size_type_node, fold_offsetof_1 (expr));
>  }
>
> -/* Warn for A ?: C expressions (with B omitted) where A is a boolean
> +/* Warn for A ?: C expressions (with B omitted) where A is a boolean
>     expression, because B will always be true. */
>
>  void
> -warn_for_omitted_condop (location_t location, tree cond)
> -{
> -  if (truth_value_p (TREE_CODE (cond)))
> -      warning_at (location, OPT_Wparentheses,
> +warn_for_omitted_condop (location_t location, tree cond)
> +{
> +  /* In C++ template declarations it can happen that the type is dependent
> +     and not yet known, thus TREE_TYPE (cond) == NULL_TREE.  */
> +  if (truth_value_p (TREE_CODE (cond))
> +      || (TREE_TYPE (cond) != NULL_TREE &&
> +	  TREE_CODE (TREE_TYPE (cond)) == BOOLEAN_TYPE))
THe "&&" at the end of the second condition goes on the next line.


With those two nits fixed, this is fine for the trunk.

jeff