From patchwork Tue Apr 21 18:14:07 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?b?TWFudWVsIEzDs3Blei1JYsOhw7Fleg==?= X-Patchwork-Id: 463450 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 6912A14012F for ; Wed, 22 Apr 2015 04:14:26 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=n52BNBD4; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=Wr96UV4bQB/Xreaun dzZvjZ+vGulOp6onigkzCKI75kvi8yCS0lIoUdZTJKjKs6vweOe9OJM3eZfq1LV/ W8+f9oDS9oCknGeSeZ8aAHOoT3uPkMO1yj8geNyfj9FFyr+zvMb2bjXfKawmUJJv 5W/z3+S7PA4ST7e+Us7+6Y+e84= 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 :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=xjfaDqOSdQquZOFVQD2wvEW WbYE=; b=n52BNBD4YgjePfoy2cQlPsdL/b1/+gO7xgf7MT/8q9DfL4cgJDxxKfy 9sP7nPLa1981znS/Q13saDwVnv88qa5p3wz+BttjmVYbLubg0qKLGOXo3ljhCGDc tMHksQW+3QMCbIK7zfkd3ti7k9ovzR8XjUIufelH/8AZm3a5dDHc= Received: (qmail 112966 invoked by alias); 21 Apr 2015 18:14:18 -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 112911 invoked by uid 89); 21 Apr 2015 18:14:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL, BAYES_50, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wg0-f50.google.com Received: from mail-wg0-f50.google.com (HELO mail-wg0-f50.google.com) (74.125.82.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 21 Apr 2015 18:14:12 +0000 Received: by wgyo15 with SMTP id o15so222615500wgy.2 for ; Tue, 21 Apr 2015 11:14:09 -0700 (PDT) X-Received: by 10.194.235.71 with SMTP id uk7mr43803464wjc.13.1429640049308; Tue, 21 Apr 2015 11:14:09 -0700 (PDT) Received: from [164.15.10.85] (euclides.ulb.ac.be. [164.15.10.85]) by mx.google.com with ESMTPSA id i6sm3689079wjf.29.2015.04.21.11.14.08 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 21 Apr 2015 11:14:08 -0700 (PDT) Message-ID: <5536936F.8090004@gmail.com> Date: Tue, 21 Apr 2015 20:14:07 +0200 From: =?windows-1252?Q?Manuel_L=F3pez-Ib=E1=F1ez?= User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: David Malcolm , Mike Stump CC: gcc-patches@gcc.gnu.org Subject: Re: Re: [RFC stage 1] Proposed new warning: -Wmisleading-indentation References: <1429196485.32584.46.camel@surprise> <6A97A5CB-3000-4030-9E5C-BE55AE79F164@comcast.net> <1429632420.32584.134.camel@surprise> In-Reply-To: <1429632420.32584.134.camel@surprise> On 21/04/15 18:07, David Malcolm wrote: > > I have the patch working now for the C++ frontend. Am attaching the > work-in-progress (sans ChangeLog). This one (v2) bootstrapped and > regrtested on x86_64-unknown-linux-gnu (Fedora 20), with: > 63 new "PASS" results in gcc.sum > 189 new "PASS" results in g++.sum > for the new test cases (relative to a control build of r222248). > I still do not understand why you need so much complexity as I explained here: https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00830.html The attached patch passes all your tests except Wmisleading-indentation-3.c, which warns only once instead of two times (it doesn't seem a big loss to me), and Wmisleading-indentation-7.c which I did not bother to implement but it is straightforward application of the if-case to the else-case. Perhaps I'm missing something that is not reflected in your tests? BTW, the start-up cost of GCC is not negligible, thus grouping similar testcases in a single file may pay off in the long term. Many small files also tend to slow down VC tools. It also makes harder to see what is tested and what is missing. Cheers, Manuel. Index: c/c-parser.c =================================================================== --- c/c-parser.c (revision 222087) +++ c/c-parser.c (working copy) @@ -5174,20 +5174,34 @@ c_parser_c99_block_statement (c_parser * location_t loc = c_parser_peek_token (parser)->location; c_parser_statement (parser); return c_end_compound_stmt (loc, block, flag_isoc99); } +static void +warn_for_misleading_indentation (location_t guard_loc, location_t body_loc, location_t next_stmt_loc, + const char *s) +{ + if (LOCATION_FILE (next_stmt_loc) == LOCATION_FILE (body_loc) + && (LOCATION_LINE (next_stmt_loc) == LOCATION_LINE (body_loc) + || (LOCATION_LINE (next_stmt_loc) > LOCATION_LINE (body_loc) + && LOCATION_COLUMN (next_stmt_loc) == LOCATION_COLUMN (body_loc)))) + if (warning_at (next_stmt_loc, OPT_Wmisleading_indentation, + "statement is indented as if it were guarded by...")) + inform (guard_loc, + "...this %qs clause, but it is not", s); +} + /* Parse the body of an if statement. This is just parsing a statement but (a) it is a block in C99, (b) we track whether the body is an if statement for the sake of -Wparentheses warnings, (c) we handle an empty body specially for the sake of -Wempty-body warnings, and (d) we call parser_compound_statement directly because c_parser_statement_after_labels resets parser->in_if_block. */ static tree -c_parser_if_body (c_parser *parser, bool *if_p) +c_parser_if_body (c_parser *parser, bool *if_p, location_t if_loc) { tree block = c_begin_compound_stmt (flag_isoc99); location_t body_loc = c_parser_peek_token (parser)->location; c_parser_all_labels (parser); *if_p = c_parser_next_token_is_keyword (parser, RID_IF); @@ -5201,11 +5215,16 @@ c_parser_if_body (c_parser *parser, bool "suggest braces around empty body in an % statement"); } else if (c_parser_next_token_is (parser, CPP_OPEN_BRACE)) add_stmt (c_parser_compound_statement (parser)); else - c_parser_statement_after_labels (parser); + { + c_parser_statement_after_labels (parser); + if (!c_parser_next_token_is_keyword (parser, RID_ELSE)) + warn_for_misleading_indentation (if_loc, body_loc, c_parser_peek_token (parser)->location, "if"); + } + return c_end_compound_stmt (body_loc, block, flag_isoc99); } /* Parse the else body of an if statement. This is just parsing a statement but (a) it is a block in C99, (b) we handle an empty body @@ -5248,10 +5267,11 @@ c_parser_if_statement (c_parser *parser) tree first_body, second_body; bool in_if_block; tree if_stmt; gcc_assert (c_parser_next_token_is_keyword (parser, RID_IF)); + location_t if_loc = c_parser_peek_token (parser)->location; c_parser_consume_token (parser); block = c_begin_compound_stmt (flag_isoc99); loc = c_parser_peek_token (parser)->location; cond = c_parser_paren_condition (parser); if (flag_cilkplus && contains_cilk_spawn_stmt (cond)) @@ -5259,11 +5279,11 @@ c_parser_if_statement (c_parser *parser) error_at (loc, "if statement cannot contain %"); cond = error_mark_node; } in_if_block = parser->in_if_block; parser->in_if_block = true; - first_body = c_parser_if_body (parser, &first_if); + first_body = c_parser_if_body (parser, &first_if, if_loc); parser->in_if_block = in_if_block; if (c_parser_next_token_is_keyword (parser, RID_ELSE)) { c_parser_consume_token (parser); second_body = c_parser_else_body (parser); @@ -5344,10 +5364,11 @@ static void c_parser_while_statement (c_parser *parser, bool ivdep) { tree block, cond, body, save_break, save_cont; location_t loc; gcc_assert (c_parser_next_token_is_keyword (parser, RID_WHILE)); + location_t while_loc = c_parser_peek_token (parser)->location; c_parser_consume_token (parser); block = c_begin_compound_stmt (flag_isoc99); loc = c_parser_peek_token (parser)->location; cond = c_parser_paren_condition (parser); if (check_no_cilk (cond, @@ -5360,11 +5381,17 @@ c_parser_while_statement (c_parser *pars annot_expr_ivdep_kind)); save_break = c_break_label; c_break_label = NULL_TREE; save_cont = c_cont_label; c_cont_label = NULL_TREE; + + location_t body_loc = UNKNOWN_LOCATION; + if (c_parser_peek_token (parser)->type != CPP_OPEN_BRACE) + body_loc = c_parser_peek_token (parser)->location; body = c_parser_c99_block_statement (parser); + warn_for_misleading_indentation (while_loc, body_loc, c_parser_peek_token (parser)->location, "while"); + c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true); add_stmt (c_end_compound_stmt (loc, block, flag_isoc99)); c_break_label = save_break; c_cont_label = save_cont; } @@ -5638,11 +5665,17 @@ c_parser_for_statement (c_parser *parser } save_break = c_break_label; c_break_label = NULL_TREE; save_cont = c_cont_label; c_cont_label = NULL_TREE; + + location_t body_loc = UNKNOWN_LOCATION; + if (c_parser_peek_token (parser)->type != CPP_OPEN_BRACE) + body_loc = c_parser_peek_token (parser)->location; body = c_parser_c99_block_statement (parser); + warn_for_misleading_indentation (for_loc, body_loc, c_parser_peek_token (parser)->location, "for"); + if (is_foreach_statement) objc_finish_foreach_loop (loc, object_expression, collection_expression, body, c_break_label, c_cont_label); else c_finish_loop (loc, cond, incr, body, c_break_label, c_cont_label, true); add_stmt (c_end_compound_stmt (loc, block, flag_isoc99 || c_dialect_objc ()));