[PATCH] Extend -Wint-in-bool-context to suspicious enum values (PR 77700)

[PATCH] Extend -Wint-in-bool-context to suspicious enum values (PR 77700)

[Bernd Edlinger]

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

Hi!

This extends -Wint-in-bool-context to uses of enum values in boolean
context, and fixes one place where accidentally an enum value was
passed to a bool parameter.

I excluded enum values 0 and 1 because that is used in
gimple-ssa-strength-reduction.c, where we have enums
which are passed in bool function arguments:

enum stride_status
{
   UNKNOWN_STRIDE = 0,
   KNOWN_STRIDE = 1
};

enum phi_adjust_status
{
   NOT_PHI_ADJUST = 0,
   PHI_ADJUST = 1
};

enum count_phis_status
{
   DONT_COUNT_PHIS = 0,
   COUNT_PHIS = 1
};

I would'nt use an enum in that way, but I think it is
at least not completely wrong to do it like that...


Unfortunately C is less strict with enum values, and from
and enum we only see an integer value without an enum type
in C.

Therefore this warning does not work in C, only in C++.
Also integer constants do not have a source location, so
the displayed location is usually a bit vague.
But I think it is still better than no warning at all.


Bootstrapped 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-bool-context3.diff --]
[-- Type: text/x-patch; name="patch-bool-context3.diff", Size: 2526 bytes --]

c-family:
2016-10-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c++/77700
	* c-common.c (c_common_truthvalue_conversion): Warn also for
	suspicious enum values in boolean context.

cp:
2016-10-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c++/77700
	* parser.c (cp_parser_base_specifier): Fix a warning.


testsuite:
2016-10-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c++/77700
	* c-c++-common/Wint-in-bool-context.c: Update test.

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 240838)
+++ gcc/c-family/c-common.c	(working copy)
@@ -4588,6 +4588,11 @@ c_common_truthvalue_conversion (location_t locatio
       return expr;
 
     case INTEGER_CST:
+      if (TREE_CODE (TREE_TYPE (expr)) == ENUMERAL_TYPE
+	  && !integer_zerop (expr)
+	  && !integer_onep (expr))
+	warning_at (location, OPT_Wint_in_bool_context,
+		    "enum constant in boolean context");
       return integer_zerop (expr) ? truthvalue_false_node
 				  : truthvalue_true_node;
 
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 240838)
+++ gcc/cp/parser.c	(working copy)
@@ -23334,7 +23334,7 @@ cp_parser_base_specifier (cp_parser* parser)
   cp_parser_nested_name_specifier_opt (parser,
 				       /*typename_keyword_p=*/true,
 				       /*check_dependency_p=*/true,
-				       typename_type,
+				       /*type_p=*/true,
 				       /*is_declaration=*/true);
   /* If the base class is given by a qualified name, assume that names
      we see are type names or templates, as appropriate.  */
Index: gcc/testsuite/c-c++-common/Wint-in-bool-context.c
===================================================================
--- gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(revision 240838)
+++ gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(working copy)
@@ -2,6 +2,8 @@
 /* { dg-options "-Wint-in-bool-context" } */
 /* { dg-do compile } */
 
+enum truth { yes, no, maybe };
+
 int foo (int a, int b)
 {
   if (a > 0 && a <= (b == 1) ? 1 : 2) /* { dg-warning "boolean context" } */
@@ -27,5 +29,11 @@ int foo (int a, int b)
 
   for (a = 0; 1 << a; a++); /* { dg-warning "boolean context" } */
 
+  if (yes || no || maybe) /* { dg-warning "boolean context" "" { target c++ } } */
+    return 8;
+
+  if (yes || no) /* { dg-bogus "boolean context" } */
+    return 9;
+
   return 0;
 }

Re: [PATCH] Extend -Wint-in-bool-context to suspicious enum values (PR 77700)

[Jason Merrill]

OK.

On Fri, Oct 7, 2016 at 11:18 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi!
>
> This extends -Wint-in-bool-context to uses of enum values in boolean
> context, and fixes one place where accidentally an enum value was
> passed to a bool parameter.
>
> I excluded enum values 0 and 1 because that is used in
> gimple-ssa-strength-reduction.c, where we have enums
> which are passed in bool function arguments:
>
> enum stride_status
> {
>    UNKNOWN_STRIDE = 0,
>    KNOWN_STRIDE = 1
> };
>
> enum phi_adjust_status
> {
>    NOT_PHI_ADJUST = 0,
>    PHI_ADJUST = 1
> };
>
> enum count_phis_status
> {
>    DONT_COUNT_PHIS = 0,
>    COUNT_PHIS = 1
> };
>
> I would'nt use an enum in that way, but I think it is
> at least not completely wrong to do it like that...
>
>
> Unfortunately C is less strict with enum values, and from
> and enum we only see an integer value without an enum type
> in C.
>
> Therefore this warning does not work in C, only in C++.
> Also integer constants do not have a source location, so
> the displayed location is usually a bit vague.
> But I think it is still better than no warning at all.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.

Re: [PATCH] Extend -Wint-in-bool-context to suspicious enum values (PR 77700)

[Trevor Saunders]

On Fri, Oct 07, 2016 at 03:18:07PM +0000, Bernd Edlinger wrote:
> Hi!
> 
> This extends -Wint-in-bool-context to uses of enum values in boolean
> context, and fixes one place where accidentally an enum value was
> passed to a bool parameter.
> 
> I excluded enum values 0 and 1 because that is used in
> gimple-ssa-strength-reduction.c, where we have enums
> which are passed in bool function arguments:
> 
> enum stride_status
> {
>    UNKNOWN_STRIDE = 0,
>    KNOWN_STRIDE = 1
> };
> 
> enum phi_adjust_status
> {
>    NOT_PHI_ADJUST = 0,
>    PHI_ADJUST = 1
> };
> 
> enum count_phis_status
> {
>    DONT_COUNT_PHIS = 0,
>    COUNT_PHIS = 1
> };
> 
> I would'nt use an enum in that way, but I think it is
> at least not completely wrong to do it like that...

Using enums here seems reasonable see the discussion here
http://gcc.gnu.org/ml/gcc/2016-10/msg00006.html however I don't
understand why that code makes the argument type bool instead of the
enum type.  Here's an untested patch to change those functions to take
the enum type.  It might make sense to change naming some, but I think
this is somewhat of an improvement as it is.

Trev

diff --git a/gcc/gimple-ssa-strength-reduction.c b/gcc/gimple-ssa-strength-reduction.c
index 7b14b91..f61805c 100644
--- a/gcc/gimple-ssa-strength-reduction.c
+++ b/gcc/gimple-ssa-strength-reduction.c
@@ -2135,7 +2135,7 @@ incr_vec_index (const widest_int &increment)
 static tree
 create_add_on_incoming_edge (slsr_cand_t c, tree basis_name,
 			     widest_int increment, edge e, location_t loc,
-			     bool known_stride)
+			     stride_status known_stride)
 {
   basic_block insert_bb;
   gimple_stmt_iterator gsi;
@@ -2151,7 +2151,7 @@ create_add_on_incoming_edge (slsr_cand_t c, tree basis_name,
   basis_type = TREE_TYPE (basis_name);
   lhs = make_temp_ssa_name (basis_type, NULL, "slsr");
 
-  if (known_stride)
+  if (known_stride == KNOWN_STRIDE)
     {
       tree bump_tree;
       enum tree_code code = PLUS_EXPR;
@@ -2218,7 +2218,7 @@ create_add_on_incoming_edge (slsr_cand_t c, tree basis_name,
 
 static tree
 create_phi_basis (slsr_cand_t c, gimple *from_phi, tree basis_name,
-		  location_t loc, bool known_stride)
+		  location_t loc, stride_status known_stride)
 {
   int i;
   tree name, phi_arg;
@@ -2451,7 +2451,8 @@ count_candidates (slsr_cand_t c)
    candidates with the same increment, also record T_0 for subsequent use.  */
 
 static void
-record_increment (slsr_cand_t c, widest_int increment, bool is_phi_adjust)
+record_increment (slsr_cand_t c, widest_int increment,
+		  phi_adjust_status is_phi_adjust)
 {
   bool found = false;
   unsigned i;
@@ -2491,7 +2492,8 @@ record_increment (slsr_cand_t c, widest_int increment, bool is_phi_adjust)
 	 the count to zero.  We're only processing it so it can possibly
 	 provide an initializer for other candidates.  */
       incr_vec[incr_vec_len].incr = increment;
-      incr_vec[incr_vec_len].count = c->basis || is_phi_adjust ? 1 : 0;
+      incr_vec[incr_vec_len].count
+       	= c->basis || (is_phi_adjust == PHI_ADJUST) ? 1 : 0;
       incr_vec[incr_vec_len].cost = COST_INFINITE;
       
       /* Optimistically record the first occurrence of this increment
@@ -2500,7 +2502,7 @@ record_increment (slsr_cand_t c, widest_int increment, bool is_phi_adjust)
          Exception:  increments of -1, 0, 1 never need initializers;
 	 and phi adjustments don't ever provide initializers.  */
       if (c->kind == CAND_ADD
-	  && !is_phi_adjust
+	  && is_phi_adjust == NOT_PHI_ADJUST
 	  && c->index == increment
 	  && (increment > 1 || increment < -1)
 	  && (gimple_assign_rhs_code (c->cand_stmt) == PLUS_EXPR
@@ -2702,7 +2704,7 @@ optimize_cands_for_speed_p (slsr_cand_t c)
 
 static int
 lowest_cost_path (int cost_in, int repl_savings, slsr_cand_t c,
-		  const widest_int &incr, bool count_phis)
+		  const widest_int &incr, count_phis_status count_phis)
 {
   int local_cost, sib_cost, savings = 0;
   widest_int cand_incr = cand_abs_increment (c);
@@ -2714,7 +2716,7 @@ lowest_cost_path (int cost_in, int repl_savings, slsr_cand_t c,
   else
     local_cost = cost_in - c->dead_savings;
 
-  if (count_phis
+  if (count_phis == COUNT_PHIS
       && phi_dependent_cand_p (c)
       && !cand_already_replaced (c))
     {
@@ -2749,7 +2751,7 @@ lowest_cost_path (int cost_in, int repl_savings, slsr_cand_t c,
 
 static int
 total_savings (int repl_savings, slsr_cand_t c, const widest_int &incr,
-	       bool count_phis)
+	       count_phis_status count_phis)
 {
   int savings = 0;
   widest_int cand_incr = cand_abs_increment (c);
@@ -2757,7 +2759,7 @@ total_savings (int repl_savings, slsr_cand_t c, const widest_int &incr,
   if (incr == cand_incr && !cand_already_replaced (c))
     savings += repl_savings + c->dead_savings;
 
-  if (count_phis
+  if (count_phis == COUNT_PHIS
       && phi_dependent_cand_p (c)
       && !cand_already_replaced (c))
     {

Re: [PATCH] Extend -Wint-in-bool-context to suspicious enum values (PR 77700)

[Jeff Law]

On 10/08/2016 04:50 PM, Trevor Saunders wrote:
> On Fri, Oct 07, 2016 at 03:18:07PM +0000, Bernd Edlinger wrote:
>> Hi!
>>
>> This extends -Wint-in-bool-context to uses of enum values in boolean
>> context, and fixes one place where accidentally an enum value was
>> passed to a bool parameter.
>>
>> I excluded enum values 0 and 1 because that is used in
>> gimple-ssa-strength-reduction.c, where we have enums
>> which are passed in bool function arguments:
>>
>> enum stride_status
>> {
>>    UNKNOWN_STRIDE = 0,
>>    KNOWN_STRIDE = 1
>> };
>>
>> enum phi_adjust_status
>> {
>>    NOT_PHI_ADJUST = 0,
>>    PHI_ADJUST = 1
>> };
>>
>> enum count_phis_status
>> {
>>    DONT_COUNT_PHIS = 0,
>>    COUNT_PHIS = 1
>> };
>>
>> I would'nt use an enum in that way, but I think it is
>> at least not completely wrong to do it like that...
>
> Using enums here seems reasonable see the discussion here
> http://gcc.gnu.org/ml/gcc/2016-10/msg00006.html however I don't
> understand why that code makes the argument type bool instead of the
> enum type.  Here's an untested patch to change those functions to take
> the enum type.  It might make sense to change naming some, but I think
> this is somewhat of an improvement as it is.
>
> Trev
>
> diff --git a/gcc/gimple-ssa-strength-reduction.c b/gcc/gimple-ssa-strength-reduction.c
> index 7b14b91..f61805c 100644
> --- a/gcc/gimple-ssa-strength-reduction.c
> +++ b/gcc/gimple-ssa-strength-reduction.c
[ ... ]
Seems reasonable.  Feel free to commit after the usual testing.

jeff

Re: [PATCH] Extend -Wint-in-bool-context to suspicious enum values (PR 77700)

[Markus Trippelsdorf]

On 2016.10.07 at 15:18 +0000, Bernd Edlinger wrote:
> Hi!
>
> This extends -Wint-in-bool-context to uses of enum values in boolean
> context, and fixes one place where accidentally an enum value was
> passed to a bool parameter.
>
> I excluded enum values 0 and 1 because that is used in
> gimple-ssa-strength-reduction.c, where we have enums
> which are passed in bool function arguments:
>
> enum stride_status
> {
>    UNKNOWN_STRIDE = 0,
>    KNOWN_STRIDE = 1
> };
>
> enum phi_adjust_status
> {
>    NOT_PHI_ADJUST = 0,
>    PHI_ADJUST = 1
> };
>
> enum count_phis_status
> {
>    DONT_COUNT_PHIS = 0,
>    COUNT_PHIS = 1
> };
>
> I would'nt use an enum in that way, but I think it is
> at least not completely wrong to do it like that...
>
>
> Unfortunately C is less strict with enum values, and from
> and enum we only see an integer value without an enum type
> in C.
>
> Therefore this warning does not work in C, only in C++.
> Also integer constants do not have a source location, so
> the displayed location is usually a bit vague.
> But I think it is still better than no warning at all.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

I came across an borderline example in Chromium today:

 62 enum FlushBehavior {
 63   // More bytes are coming, don't flush the codec.
 64   DoNotFlush = 0,
 65
 66   // A fetch has hit EOF. Some codecs handle fetches differently, for compat
 67   // reasons.
 68   FetchEOF,
 69
 70   // Do a full flush of the codec.
 71   DataEOF
 72 };
 73
 74 static_assert(!DoNotFlush, "DoNotFlush should be falsy");
 75 static_assert(FetchEOF, "FetchEOF should be truthy");
 76 static_assert(DataEOF, "DataEOF should be truthy");

../../third_party/WebKit/Source/wtf/text/TextCodec.h:76:51: warning: enum constant in boolean context [-Wint-in-bool-context]
 static_assert(DataEOF, "DataEOF should be truthy");
                                                   ^

--
Markus