diff mbox

C++ PATCH to forbid use of bool with the ++ operator

Message ID 20160913183633.GO19950@redhat.com
State New
Headers show

Commit Message

Marek Polacek Sept. 13, 2016, 6:36 p.m. UTC 
For historical reasons, incrementing a boolean was allowed (because anything
non-0 is true), while decrementing is forbidden.  This has now changed, and
since C++17 even ++ on bool is invalid.  And C++ 03/11/14 [depr.incr.bool]
says that ++ is deprecated, but we never diagnosed this.

A bunch of testcases needed updating, but nothing unexpected.

(Even though I recently changed out documentation to use "boolean" rather
than "Boolean", I'm keeping this as it is.  I can tweak that in a follow
up.)

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

2016-09-13  Marek Polacek  <polacek@redhat.com>

	* typeck.c (cp_build_unary_op): Diagnose incrementing boolean
	expressions.

	* c-c++-common/gomp/atomic-12.c: Use -Wno-deprecated.
	* c-c++-common/gomp/atomic-13.c: Likewise.
	* c-c++-common/gomp/atomic-14.c: Likewise.
	* g++.dg/cpp1y/lambda-init11.C: Remove invalid code.
	* g++.dg/cpp1z/bool-increment1.C: New test.
	* c-c++-common/pr60439.c: Add dg-warning.
	* g++.dg/expr/bitfield4.C: Likewise.
	* g++.dg/expr/bitfield5.C: Likewise.
	* g++.dg/expr/bitfield6.C: Likewise.
	* g++.dg/expr/bool1.C: Likewise.
	* g++.dg/expr/bool3.C: Likewise.
	* g++.dg/expr/lval3.C: Likewise.
	* g++.dg/expr/lval4.C: Likewise.
	* g++.old-deja/g++.jason/bool5.C: Likewise.


	Marek

Comments

Martin Sebor Sept. 13, 2016, 7:55 p.m. UTC | #1 
> -	/* Forbid using -- on `bool'.  */
> +	/* Forbid using -- or ++ in C++17 on `bool'.  */
>   	if (TREE_CODE (declared_type) == BOOLEAN_TYPE)
>   	  {
>   	    if (code == POSTDECREMENT_EXPR || code == PREDECREMENT_EXPR)
> @@ -6040,6 +6040,20 @@ cp_build_unary_op (enum tree_code code, tree xarg, int noconvert,
>                            "to %<operator--%>");
>   		return error_mark_node;
>   	      }
> +	    else
> +	      {
> +		if (cxx_dialect >= cxx1z)
> +		  {
> +		    if (complain & tf_error)
> +		      error ("use of Boolean expression as operand "
> +			     "to %<operator++%> is forbidden in C++1z");

The capitalization of Boolean here caught my eye because it's
inconsistent with the recent spelling adopted in the documentation.
(It's also missing an article "a Boolean expression," although
dropping those is common in diagnostics. Still, it would be nice
to have a guideline/convention and use it consistently.)

Back to Boolean, I was actually going to comment on the Boolean
-> boolean change and suggest going in the opposite direction but
in the end decided not to (as Sandra's links showed, there's support
for both).

But having seen Boolean capitalized here I have changed my mind
again. I'd like to (belatedly) speak up in support of Boolean
(though I feel less strongly about it than I do about consistency).

FWIW, I've always found the capitalized spelling more appropriate
than the lowercase form. There are many examples of it, including
the Wikipedia entries on Boolean algebra, Boolean expression, and
Boolean data type, compiler manuals including those of HP C and
aCC (though HP is inconsistent and uses both), the IBM XLC/C++
Language Reference, the Microsoft Visual Basic documentation of
the type, the Wolfram MathWorld article on Boolean Algebra, and
so on.

Having said all that, since this is C++ the message could and
arguably should refer to a bool expression (or type) instead
and avoid having to deal with this altogether. In fact, it
would be simpler to rephrase the message as:

   "use of an operand of type %qT in ... is deprecated",
   boolean_type_node

As a separate issue, the message hardcodes operator++ but
the comment farther above says:

    Forbid using -- or ++ in C++17 on `bool'

Should it be parameterized on the kind expression and both
expressions tested?

Martin
Jason Merrill Sept. 14, 2016, 3:37 a.m. UTC | #2 
On Tue, Sep 13, 2016 at 3:55 PM, Martin Sebor <msebor@gmail.com> wrote:
> Having said all that, since this is C++ the message could and
> arguably should refer to a bool expression (or type) instead
> and avoid having to deal with this altogether. In fact, it
> would be simpler to rephrase the message as:
>
>   "use of an operand of type %qT in ... is deprecated",
>   boolean_type_node

Yes.

> As a separate issue, the message hardcodes operator++ but
> the comment farther above says:
>
>    Forbid using -- or ++ in C++17 on `bool'
>
> Should it be parameterized on the kind expression and both
> expressions tested?

The code immediately above the addition handles --.

Jason
Marek Polacek Sept. 14, 2016, 1:08 p.m. UTC | #3 
On Tue, Sep 13, 2016 at 01:55:56PM -0600, Martin Sebor wrote:
> > -	/* Forbid using -- on `bool'.  */
> > +	/* Forbid using -- or ++ in C++17 on `bool'.  */
> >   	if (TREE_CODE (declared_type) == BOOLEAN_TYPE)
> >   	  {
> >   	    if (code == POSTDECREMENT_EXPR || code == PREDECREMENT_EXPR)
> > @@ -6040,6 +6040,20 @@ cp_build_unary_op (enum tree_code code, tree xarg, int noconvert,
> >                            "to %<operator--%>");
> >   		return error_mark_node;
> >   	      }
> > +	    else
> > +	      {
> > +		if (cxx_dialect >= cxx1z)
> > +		  {
> > +		    if (complain & tf_error)
> > +		      error ("use of Boolean expression as operand "
> > +			     "to %<operator++%> is forbidden in C++1z");
> 
> The capitalization of Boolean here caught my eye because it's
> inconsistent with the recent spelling adopted in the documentation.
> (It's also missing an article "a Boolean expression," although
> dropping those is common in diagnostics. Still, it would be nice
> to have a guideline/convention and use it consistently.)
> 
> Back to Boolean, I was actually going to comment on the Boolean
> -> boolean change and suggest going in the opposite direction but
> in the end decided not to (as Sandra's links showed, there's support
> for both).
> 
> But having seen Boolean capitalized here I have changed my mind
> again. I'd like to (belatedly) speak up in support of Boolean
> (though I feel less strongly about it than I do about consistency).
 
Well, my point was to get rid of this inconsistency, I don't really
care whether it's Boolean or boolean.  But since boolean was used
most of the time, I went with that.

	Marek
Jason Merrill Sept. 14, 2016, 2:54 p.m. UTC | #4 
On Wed, Sep 14, 2016 at 9:08 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Tue, Sep 13, 2016 at 01:55:56PM -0600, Martin Sebor wrote:
>> > -   /* Forbid using -- on `bool'.  */
>> > +   /* Forbid using -- or ++ in C++17 on `bool'.  */
>> >     if (TREE_CODE (declared_type) == BOOLEAN_TYPE)
>> >       {
>> >         if (code == POSTDECREMENT_EXPR || code == PREDECREMENT_EXPR)
>> > @@ -6040,6 +6040,20 @@ cp_build_unary_op (enum tree_code code, tree xarg, int noconvert,
>> >                            "to %<operator--%>");
>> >             return error_mark_node;
>> >           }
>> > +       else
>> > +         {
>> > +           if (cxx_dialect >= cxx1z)
>> > +             {
>> > +               if (complain & tf_error)
>> > +                 error ("use of Boolean expression as operand "
>> > +                        "to %<operator++%> is forbidden in C++1z");
>>
>> The capitalization of Boolean here caught my eye because it's
>> inconsistent with the recent spelling adopted in the documentation.
>> (It's also missing an article "a Boolean expression," although
>> dropping those is common in diagnostics. Still, it would be nice
>> to have a guideline/convention and use it consistently.)
>>
>> Back to Boolean, I was actually going to comment on the Boolean
>> -> boolean change and suggest going in the opposite direction but
>> in the end decided not to (as Sandra's links showed, there's support
>> for both).
>>
>> But having seen Boolean capitalized here I have changed my mind
>> again. I'd like to (belatedly) speak up in support of Boolean
>> (though I feel less strongly about it than I do about consistency).
>
> Well, my point was to get rid of this inconsistency, I don't really
> care whether it's Boolean or boolean.  But since boolean was used
> most of the time, I went with that.

Martin's point, which I agree with, is that for C++ we want to use
"bool" rather than either of those.

Jason
Marek Polacek Sept. 14, 2016, 2:59 p.m. UTC | #5 
On Wed, Sep 14, 2016 at 10:54:35AM -0400, Jason Merrill wrote:
> Martin's point, which I agree with, is that for C++ we want to use
> "bool" rather than either of those.

Sure, I'm testing another version of this patch which has this fixed.

	Marek
diff mbox

Patch

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index a591d29..341f722 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -6030,7 +6030,7 @@  cp_build_unary_op (enum tree_code code, tree xarg, int noconvert,
                              complain))
 	  return error_mark_node;
 
-	/* Forbid using -- on `bool'.  */
+	/* Forbid using -- or ++ in C++17 on `bool'.  */
 	if (TREE_CODE (declared_type) == BOOLEAN_TYPE)
 	  {
 	    if (code == POSTDECREMENT_EXPR || code == PREDECREMENT_EXPR)
@@ -6040,6 +6040,20 @@  cp_build_unary_op (enum tree_code code, tree xarg, int noconvert,
                          "to %<operator--%>");
 		return error_mark_node;
 	      }
+	    else
+	      {
+		if (cxx_dialect >= cxx1z)
+		  {
+		    if (complain & tf_error)
+		      error ("use of Boolean expression as operand "
+			     "to %<operator++%> is forbidden in C++1z");
+		    return error_mark_node;
+		  }
+		/* Otherwise, [depr.incr.bool] says this is deprecated.  */
+		else if (!in_system_header_at (input_location))
+		  warning (OPT_Wdeprecated, "use of Boolean expression as "
+			   "operand to %<operator++%> is deprecated");
+	      }
 	    val = boolean_increment (code, arg);
 	  }
 	else if (code == POSTINCREMENT_EXPR || code == POSTDECREMENT_EXPR)
diff --git gcc/testsuite/c-c++-common/gomp/atomic-12.c gcc/testsuite/c-c++-common/gomp/atomic-12.c
index 145e420..e9ca650 100644
--- gcc/testsuite/c-c++-common/gomp/atomic-12.c
+++ gcc/testsuite/c-c++-common/gomp/atomic-12.c
@@ -1,6 +1,6 @@ 
 /* PR middle-end/45423 */
 /* { dg-do compile } */
-/* { dg-options "-fopenmp -fdump-tree-gimple -g0" } */
+/* { dg-options "-fopenmp -fdump-tree-gimple -g0 -Wno-deprecated" } */
 /* atomicvar should never be referenced in between the barrier and
    following #pragma omp atomic_load.  */
 /* { dg-final { scan-tree-dump-not "barrier\[^#\]*atomicvar" "gimple" } } */
diff --git gcc/testsuite/c-c++-common/gomp/atomic-13.c gcc/testsuite/c-c++-common/gomp/atomic-13.c
index 2452035..7f4afcf 100644
--- gcc/testsuite/c-c++-common/gomp/atomic-13.c
+++ gcc/testsuite/c-c++-common/gomp/atomic-13.c
@@ -1,6 +1,6 @@ 
 /* PR middle-end/45423 */
 /* { dg-do compile } */
-/* { dg-options "-fopenmp -fdump-tree-gimple -g0 -O2" } */
+/* { dg-options "-fopenmp -fdump-tree-gimple -g0 -O2 -Wno-deprecated" } */
 /* atomicvar should never be referenced in between the barrier and
    following #pragma omp atomic_load.  */
 /* { dg-final { scan-tree-dump-not "barrier\[^#\]*atomicvar" "gimple" } } */
diff --git gcc/testsuite/c-c++-common/gomp/atomic-14.c gcc/testsuite/c-c++-common/gomp/atomic-14.c
index f8fc9d8..7e23453 100644
--- gcc/testsuite/c-c++-common/gomp/atomic-14.c
+++ gcc/testsuite/c-c++-common/gomp/atomic-14.c
@@ -1,6 +1,6 @@ 
 /* PR middle-end/45423 */
 /* { dg-do compile } */
-/* { dg-options "-fopenmp" } */
+/* { dg-options "-fopenmp -Wno-deprecated" } */
 
 #ifdef __cplusplus
 bool *baz ();
diff --git gcc/testsuite/c-c++-common/pr60439.c gcc/testsuite/c-c++-common/pr60439.c
index 68bd33c..71b397a 100644
--- gcc/testsuite/c-c++-common/pr60439.c
+++ gcc/testsuite/c-c++-common/pr60439.c
@@ -132,6 +132,7 @@  f6 (bool b)
       break;
     }
   switch (b++) /* { dg-warning "switch condition has" } */
+  /* { dg-warning "is deprecated" "" { target c++ } 134 } */
     {
     case 3:
       break;
diff --git gcc/testsuite/g++.dg/cpp1y/lambda-init11.C gcc/testsuite/g++.dg/cpp1y/lambda-init11.C
index f7525d8..4d434cd 100644
--- gcc/testsuite/g++.dg/cpp1y/lambda-init11.C
+++ gcc/testsuite/g++.dg/cpp1y/lambda-init11.C
@@ -16,5 +16,4 @@  int main(){
   foo(3.14f);
   foo(0);
   foo('a');
-  foo(false);
 }
diff --git gcc/testsuite/g++.dg/expr/bitfield4.C gcc/testsuite/g++.dg/expr/bitfield4.C
index d824964..7fae086 100644
--- gcc/testsuite/g++.dg/expr/bitfield4.C
+++ gcc/testsuite/g++.dg/expr/bitfield4.C
@@ -14,6 +14,6 @@  template <>
 void f(bool) {} 
 
 int main() {
-  f(s.x++);
-  f(++s.x);
+  f(s.x++); // { dg-warning "deprecated" }
+  f(++s.x); // { dg-warning "deprecated" }
 }
diff --git gcc/testsuite/g++.dg/expr/bitfield5.C gcc/testsuite/g++.dg/expr/bitfield5.C
index 3d18e15..0a37f9f 100644
--- gcc/testsuite/g++.dg/expr/bitfield5.C
+++ gcc/testsuite/g++.dg/expr/bitfield5.C
@@ -8,10 +8,10 @@  struct S {
 S s;
 
 int main() {
-  s.x++;
+  s.x++; // { dg-warning "deprecated" }
   if (s.x != 1)
     return 1;
-  ++s.x;
+  ++s.x; // { dg-warning "deprecated" }
   if (s.x != 1)
     return 2;
 }
diff --git gcc/testsuite/g++.dg/expr/bitfield6.C gcc/testsuite/g++.dg/expr/bitfield6.C
index 6f6d559..8523866 100644
--- gcc/testsuite/g++.dg/expr/bitfield6.C
+++ gcc/testsuite/g++.dg/expr/bitfield6.C
@@ -7,5 +7,5 @@  struct S {
 S s;
 
 void f() {
-  ++s.x = false;
+  ++s.x = false; // { dg-warning "deprecated" }
 }
diff --git gcc/testsuite/g++.dg/expr/bool1.C gcc/testsuite/g++.dg/expr/bool1.C
index bfb40e3..503e8b4 100644
--- gcc/testsuite/g++.dg/expr/bool1.C
+++ gcc/testsuite/g++.dg/expr/bool1.C
@@ -10,8 +10,8 @@  int main()
   my_bool b = false;
   int i;
 
-  b++;
-  b++;
+  b++; // { dg-warning "deprecated" }
+  b++; // { dg-warning "deprecated" }
   i = b;
   if (i != 1)
     abort ();
diff --git gcc/testsuite/g++.dg/expr/bool3.C gcc/testsuite/g++.dg/expr/bool3.C
index 61669e2..1866ed4 100644
--- gcc/testsuite/g++.dg/expr/bool3.C
+++ gcc/testsuite/g++.dg/expr/bool3.C
@@ -10,8 +10,8 @@  int main()
   my_bool b = false;
   int i;
 
-  b++;
-  b++;
+  b++; // { dg-warning "deprecated" }
+  b++; // { dg-warning "deprecated" }
   i = b;
   if (i != 1)
     abort ();
diff --git gcc/testsuite/g++.dg/expr/lval3.C gcc/testsuite/g++.dg/expr/lval3.C
index f106e69..8e0aead 100644
--- gcc/testsuite/g++.dg/expr/lval3.C
+++ gcc/testsuite/g++.dg/expr/lval3.C
@@ -4,6 +4,7 @@  f()
 {
   bool i = 0;
   i++ = 3; // { dg-error "" }
+  // { dg-warning "deprecated" "" { target *-*-* } 6 }
 }
 
 
diff --git gcc/testsuite/g++.dg/expr/lval4.C gcc/testsuite/g++.dg/expr/lval4.C
index c66e2f6..b903ec8 100644
--- gcc/testsuite/g++.dg/expr/lval4.C
+++ gcc/testsuite/g++.dg/expr/lval4.C
@@ -4,6 +4,7 @@  f()
 {
   bool i = 0;
   ++i = 3;
+  // { dg-warning "deprecated" "" { target *-*-* } 6 }
 }
 
 
diff --git gcc/testsuite/g++.old-deja/g++.jason/bool5.C gcc/testsuite/g++.old-deja/g++.jason/bool5.C
index 1d2f5b6..0a16ccb 100644
--- gcc/testsuite/g++.old-deja/g++.jason/bool5.C
+++ gcc/testsuite/g++.old-deja/g++.jason/bool5.C
@@ -2,10 +2,10 @@ 
 int main ()
 {
   bool b = false;
-  int i = b++;
+  int i = b++; // { dg-warning "deprecated" }
   if (i != false || b != true)
     return 1;
-  i = b++;
+  i = b++; // { dg-warning "deprecated" }
   if (i != true || b != true)
     return 1;
 }