From patchwork Tue Jun 3 14:57:22 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 355570 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 56F5814007C for ; Wed, 4 Jun 2014 00:59:40 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=cxmOfDTxLtbrjrdMw lAzXoxtLk0YJWDg0uJkVJFSqjSIaxd6FEsRqY7sExOAiejlm2nb/67IrxJ9RopyH jOJp9HKWJVRCfgqsYK1HcExKbf2DNNniSGgS5PCZScYYGGTf9P/2MtFakH3NT0pf rUEVvHRJlGJv8U2IiXAuHNr1Gs= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=VQGoB1r9gZHd9/zPh7kFHtw l6hM=; b=B3T61sWc1nOGpRve0KVXLvIk+hZLT+5vOJyXOWYb3uywE8XSUwuctOi 73TAX6i04kfd3wsUHRucaQV4VsRgA094wKEx9NWcToSEZKuVsUh3mx+Vkyocrwk8 0IdZFf3oO9eAwrLAz81/uKLFK0XsGNWJYXnvoGyKGkDcsBVueNNM= Received: (qmail 11900 invoked by alias); 3 Jun 2014 14:57:34 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 11845 invoked by uid 89); 3 Jun 2014 14:57:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Jun 2014 14:57:31 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s53EvT7v027328 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 3 Jun 2014 10:57:29 -0400 Received: from redhat.com (ovpn-116-106.ams2.redhat.com [10.36.116.106]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s53EvNFR015492 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Tue, 3 Jun 2014 10:57:26 -0400 Date: Tue, 3 Jun 2014 16:57:22 +0200 From: Marek Polacek To: Jason Merrill Cc: Jeff Law , Steven Bosscher , GCC Patches , "Joseph S. Myers" Subject: Re: [C PATCH] Warn if switch has boolean value (PR c/60439) Message-ID: <20140603145722.GC29196@redhat.com> References: <20140418053021.GH20332@redhat.com> <20140418115013.GK20332@redhat.com> <53632870.6030503@redhat.com> <20140524080048.GF17600@redhat.com> <538CF3E4.4010304@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <538CF3E4.4010304@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) On Mon, Jun 02, 2014 at 06:00:04PM -0400, Jason Merrill wrote: > On 05/24/2014 04:00 AM, Marek Polacek wrote: > >+ /* Warn if the condition has boolean value. */ > >+ tree e = cond; > >+ while (TREE_CODE (e) == COMPOUND_EXPR) > >+ e = TREE_OPERAND (e, 1); > >+ > >+ if (TREE_CODE (orig_type) == BOOLEAN_TYPE > >+ || (truth_value_p (TREE_CODE (e)) > >+ && TREE_CODE (orig_type) != INTEGER_TYPE)) > >+ warning_at (input_location, OPT_Wswitch_bool, > >+ "switch condition has boolean value"); > > For C++ it should be "type bool", not "boolean value". And it should be Ok, changed (but not for C), and adjusted the testcase. > enough to just check for BOOLEAN_TYPE, without looking through > COMPOUND_EXPR. Nice. I dropped looking through COMPOUND_EXPR. > >2) Since the warning is now enabled even for the C++ FE, it's > > exercised during bootstrap. Turned out that gengtype generates > > code like > > switch (TREE_CODE (...) == INTEGER_TYPE) { ... } > > that would mar the bootstrap - so I tweaked it to generate > > switch ((int) (TREE_CODE (...) == INTEGER_TYPE) { ... }) > > instead. Does that make sense? > > Makes sense to me. Thanks. I regtested the following, bootstrap is still in progress, but I don't expect any issues. Ok? 2014-06-03 Marek Polacek PR c/60439 * doc/invoke.texi: Document -Wswitch-bool. * function.c (stack_protect_epilogue): Cast controlling expression of the switch to int. * gengtype.c (walk_type): Generate switch expression with its controlling expression cast to int. c/ * c-parser.c (c_parser_switch_statement): Pass explicit_cast_p to c_start_case. * c-tree.h (c_start_case): Update. * c-typeck.c (c_start_case): Add new boolean parameter. Warn if switch condition has boolean value. cp/ * semantics.c (finish_switch_cond): Warn if switch condition has boolean value. c-family/ * c.opt (Wswitch-bool): New option. testsuite/ * c-c++-common/pr60439.c: New test. * g++.dg/eh/scope1.C (f4): Add dg-warning. Marek diff --git gcc/c-family/c.opt gcc/c-family/c.opt index c586e65..5d36a80 100644 --- gcc/c-family/c.opt +++ gcc/c-family/c.opt @@ -534,6 +534,10 @@ Wswitch-enum C ObjC C++ ObjC++ Var(warn_switch_enum) Warning Warn about all enumerated switches missing a specific case +Wswitch-bool +C ObjC C++ ObjC++ Warning Init(1) +Warn about switches with boolean controlling expression + Wmissing-format-attribute C ObjC C++ ObjC++ Alias(Wsuggest-attribute=format) ; diff --git gcc/c/c-parser.c gcc/c/c-parser.c index 1d9780e..abd636c 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -5197,9 +5197,13 @@ c_parser_switch_statement (c_parser *parser) gcc_assert (c_parser_next_token_is_keyword (parser, RID_SWITCH)); c_parser_consume_token (parser); block = c_begin_compound_stmt (flag_isoc99); + bool explicit_cast_p = false; if (c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) { switch_cond_loc = c_parser_peek_token (parser)->location; + if (c_parser_next_token_is (parser, CPP_OPEN_PAREN) + && c_token_starts_typename (c_parser_peek_2nd_token (parser))) + explicit_cast_p = true; ce = c_parser_expression (parser); ce = convert_lvalue_to_rvalue (switch_cond_loc, ce, true, false); expr = ce.value; @@ -5217,7 +5221,7 @@ c_parser_switch_statement (c_parser *parser) switch_cond_loc = UNKNOWN_LOCATION; expr = error_mark_node; } - c_start_case (switch_loc, switch_cond_loc, expr); + c_start_case (switch_loc, switch_cond_loc, expr, explicit_cast_p); save_break = c_break_label; c_break_label = NULL_TREE; body = c_parser_c99_block_statement (parser); diff --git gcc/c/c-tree.h gcc/c/c-tree.h index e7dcb35..133930f 100644 --- gcc/c/c-tree.h +++ gcc/c/c-tree.h @@ -614,7 +614,7 @@ extern void process_init_element (location_t, struct c_expr, bool, struct obstack *); extern tree build_compound_literal (location_t, tree, tree, bool); extern void check_compound_literal_type (location_t, struct c_type_name *); -extern tree c_start_case (location_t, location_t, tree); +extern tree c_start_case (location_t, location_t, tree, bool); extern void c_finish_case (tree); extern tree build_asm_expr (location_t, tree, tree, tree, tree, tree, bool); extern tree build_asm_stmt (tree, tree); diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c index 6ca584b..a98ce07 100644 --- gcc/c/c-typeck.c +++ gcc/c/c-typeck.c @@ -9361,12 +9361,13 @@ struct c_switch *c_switch_stack; /* Start a C switch statement, testing expression EXP. Return the new SWITCH_EXPR. SWITCH_LOC is the location of the `switch'. - SWITCH_COND_LOC is the location of the switch's condition. */ + SWITCH_COND_LOC is the location of the switch's condition. + EXPLICIT_CAST_P is true if the expression EXP has explicit cast. */ tree c_start_case (location_t switch_loc, location_t switch_cond_loc, - tree exp) + tree exp, bool explicit_cast_p) { tree orig_type = error_mark_node; struct c_switch *cs; @@ -9387,6 +9388,19 @@ c_start_case (location_t switch_loc, else { tree type = TYPE_MAIN_VARIANT (orig_type); + tree e = exp; + + /* Warn if the condition has boolean value. */ + while (TREE_CODE (e) == COMPOUND_EXPR) + e = TREE_OPERAND (e, 1); + + if ((TREE_CODE (type) == BOOLEAN_TYPE + || truth_value_p (TREE_CODE (e))) + /* Explicit cast to int suppresses this warning. */ + && !(TREE_CODE (type) == INTEGER_TYPE + && explicit_cast_p)) + warning_at (switch_cond_loc, OPT_Wswitch_bool, + "switch condition has boolean value"); if (!in_system_header_at (input_location) && (type == long_integer_type_node diff --git gcc/cp/semantics.c gcc/cp/semantics.c index 21920b4..abd668e 100644 --- gcc/cp/semantics.c +++ gcc/cp/semantics.c @@ -1130,6 +1130,13 @@ finish_switch_cond (tree cond, tree switch_stmt) orig_type = TREE_TYPE (cond); if (cond != error_mark_node) { + /* Warn if the condition has boolean value. */ + if (TREE_CODE (orig_type) == BOOLEAN_TYPE + || (truth_value_p (TREE_CODE (cond)) + && TREE_CODE (orig_type) != INTEGER_TYPE)) + warning_at (input_location, OPT_Wswitch_bool, + "switch condition has type bool"); + /* [stmt.switch] Integral promotions are performed. */ diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi index 6db0d55..1c2e079 100644 --- gcc/doc/invoke.texi +++ gcc/doc/invoke.texi @@ -271,7 +271,7 @@ Objective-C and Objective-C++ Dialects}. -Wstrict-aliasing=n @gol -Wstrict-overflow -Wstrict-overflow=@var{n} @gol -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{]} @gol -Wmissing-format-attribute @gol --Wswitch -Wswitch-default -Wswitch-enum -Wsync-nand @gol +-Wswitch -Wswitch-default -Wswitch-enum -Wswitch-bool -Wsync-nand @gol -Wsystem-headers -Wtrampolines -Wtrigraphs -Wtype-limits -Wundef @gol -Wuninitialized -Wunknown-pragmas -Wno-pragmas @gol -Wunsuffixed-float-constants -Wunused -Wunused-function @gol @@ -3846,6 +3846,22 @@ between @option{-Wswitch} and this option is that this option gives a warning about an omitted enumeration code even if there is a @code{default} label. +@item -Wswitch-bool +@opindex Wswitch-bool +@opindex Wno-switch-bool +Warn whenever a @code{switch} statement has an index of boolean type. +It is possible to suppress this warning by casting the controlling +expression to a type other than @code{bool}. For example: +@smallexample +@group +switch ((int) (a == 4)) + @{ + ... + @} +@end group +@end smallexample +This warning is enabled by default for C and C++ programs. + @item -Wsync-nand @r{(C and C++ only)} @opindex Wsync-nand @opindex Wno-sync-nand diff --git gcc/function.c gcc/function.c index ec2ea26..922f567 100644 --- gcc/function.c +++ gcc/function.c @@ -4649,7 +4649,7 @@ stack_protect_epilogue (void) /* Allow the target to compare Y with X without leaking either into a register. */ - switch (HAVE_stack_protect_test != 0) + switch ((int) (HAVE_stack_protect_test != 0)) { case 1: tmp = gen_stack_protect_test (x, y, label); diff --git gcc/gengtype.c gcc/gengtype.c index bc8f701..ffe3f94 100644 --- gcc/gengtype.c +++ gcc/gengtype.c @@ -3099,9 +3099,9 @@ walk_type (type_p t, struct walk_type_data *d) t->u.s.tag); desc = "1"; } - oprintf (d->of, "%*sswitch (", d->indent, ""); + oprintf (d->of, "%*sswitch ((int) (", d->indent, ""); output_escaped_param (d, desc, "desc"); - oprintf (d->of, ")\n"); + oprintf (d->of, "))\n"); d->indent += 2; oprintf (d->of, "%*s{\n", d->indent, ""); } @@ -3121,9 +3121,9 @@ walk_type (type_p t, struct walk_type_data *d) "missing `tag' option for type `%s'", t->u.s.tag); } - oprintf (d->of, "%*sswitch (", d->indent, ""); + oprintf (d->of, "%*sswitch ((int) (", d->indent, ""); output_escaped_param (d, desc, "desc"); - oprintf (d->of, ")\n"); + oprintf (d->of, "))\n"); d->indent += 2; oprintf (d->of, "%*s{\n", d->indent, ""); oprintf (d->of, "%*scase %s:\n", d->indent, "", type_tag); diff --git gcc/testsuite/c-c++-common/pr60439.c gcc/testsuite/c-c++-common/pr60439.c index e69de29..3368a0b 100644 --- gcc/testsuite/c-c++-common/pr60439.c +++ gcc/testsuite/c-c++-common/pr60439.c @@ -0,0 +1,108 @@ +/* PR c/60439 */ +/* { dg-do compile } */ + +#ifndef __cplusplus +# define bool _Bool +#endif + +extern bool foo (void); + +void +f1 (bool b) +{ + switch (b) /* { dg-warning "switch condition has" } */ + break; +} + +void +f2 (int a, int b) +{ + switch (a && b) /* { dg-warning "switch condition has" } */ + break; + switch ((bool) (a && b)) /* { dg-warning "switch condition has" } */ + break; + switch ((a && b) || a) /* { dg-warning "switch condition has" } */ + break; + /* No warnings on following. */ + switch ((int) (a && b)) + break; + switch ((unsigned int) (a && b)) + break; + switch ((unsigned short int) (a && b)) + break; + switch ((char) (a && b)) + break; +} + +void +f3 (int a) +{ + switch (!!a) /* { dg-warning "switch condition has" } */ + break; + switch (!a) /* { dg-warning "switch condition has" } */ + break; +} + +void +f4 (void) +{ + switch (foo ()) /* { dg-warning "switch condition has" } */ + break; +} + +void +f5 (int a) +{ + switch (a == 3) /* { dg-warning "switch condition has" } */ + break; + switch (a != 3) /* { dg-warning "switch condition has" } */ + break; + switch (a > 3) /* { dg-warning "switch condition has" } */ + break; + switch (a < 3) /* { dg-warning "switch condition has" } */ + break; + switch (a <= 3) /* { dg-warning "switch condition has" } */ + break; + switch (a >= 3) /* { dg-warning "switch condition has" } */ + break; + switch (foo (), foo (), a >= 42) /* { dg-warning "switch condition has" } */ + break; + switch (a == 3, a & 4, a ^ 5, a) + break; + switch ((int) (a == 3)) + break; + switch ((int) (a != 3)) + break; +} + +void +f6 (bool b) +{ + switch (b) /* { dg-warning "switch condition has" } */ + break; + switch (!b) /* { dg-warning "switch condition has" } */ + break; + switch (b++) /* { dg-warning "switch condition has" } */ + break; +} + +void +f7 (void) +{ + bool b; + switch (b = 1) /* { dg-warning "switch condition has" } */ + break; +} + +void +f8 (int i) +{ + switch (i) + break; + switch ((int) i) + break; + switch ((unsigned int) i) + break; + switch ((bool) i) /* { dg-warning "switch condition has" } */ + break; +} diff --git gcc/testsuite/g++.dg/eh/scope1.C gcc/testsuite/g++.dg/eh/scope1.C index 276e0d6..4884ec7 100644 --- gcc/testsuite/g++.dg/eh/scope1.C +++ gcc/testsuite/g++.dg/eh/scope1.C @@ -31,7 +31,7 @@ void f3 () void f4 () { - switch (C br = C()) + switch (C br = C()) /* { dg-warning "switch condition has" } */ { default: abort ();