From patchwork Tue Oct 3 17:08:43 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathan Sidwell X-Patchwork-Id: 820966 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-463397-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="KCu7nbwM"; dkim-atps=neutral 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 3y659D43qJz9ryr for ; Wed, 4 Oct 2017 04:09:01 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:to:cc :from:subject:message-id:date:mime-version:content-type; q=dns; s=default; b=sGRE+cPtqvG3B/1o9dgPi2vzY9hg3PHbUfIkPt8jLew8/1si6m Ff4V1+6dAC7yM/kEhINIuYm6Kfrs8sqR8ytvfcj+iMm4YuJTPD/n/I2DG5CVyw2A qNjjnnZIK4pReE5T9y4i0jd3XhlP7a49hBfVpUaebPY9VlmiKkG1xmA40= 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:to:cc :from:subject:message-id:date:mime-version:content-type; s= default; bh=8noTnfz1/AZv3cjny0ndTUgakEA=; b=KCu7nbwMsVRkNe7e+YKf pBnt4giVrrLdT0WbL2IUANILR17kKgew+UndPUvkj9cWK0WaTT6TPAHWzar+N/5F +HUepPdJMPZ8LYf1BDq9N5DO7uJkGSL1t7zbKyvdNu9OnFSC9jRKtOrzb2M0Ljz5 /ZBYPR81yXbwfBKGhf6Oaow= Received: (qmail 30107 invoked by alias); 3 Oct 2017 17:08:51 -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 30095 invoked by uid 89); 3 Oct 2017 17:08:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.3 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=poorly, streamed, unexpectedly, Signature X-HELO: mail-qt0-f182.google.com Received: from mail-qt0-f182.google.com (HELO mail-qt0-f182.google.com) (209.85.216.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Oct 2017 17:08:47 +0000 Received: by mail-qt0-f182.google.com with SMTP id 47so14184616qts.10 for ; Tue, 03 Oct 2017 10:08:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:to:cc:from:subject:message-id:date :user-agent:mime-version:content-language; bh=zR9Z+FVxifgzromsbSGZi+rIAhET2MlMDyuMCkDfAOw=; b=NslEsAhwQQQhT0zuLMXIle0giGBZ1RGuOQeUo6WZP+rVFXdrtZHZmnPshvj/L9YZor CcGnBE4A5lsyJMlCu9jNf+A3BKhFE/ZRNIIYKvvsOqgVAbMLQI2OeXKfH/9fcd6MNQ2P x7W2UJCjrInW57VvDBapgr9Q9dKa40VP/xthfkvcPCzxmoPIvxZXJ6K70bAYxYLVD4M5 0QExejaljQUbmWhI0hcKQRPv7btiGFw49bNwdzrMs9uAAti3qepYceyxgJ2sObPKF72L 7qxct2Okosrton0VHA5Xksx55+6cfjXxkBgSIj8+nD3vLGQ8tanKo6kM/71uX+tsFOnF JssA== X-Gm-Message-State: AMCzsaVWtl/awv/Tee8bONXRPcSN3GQpmOU47PhXXH1FSeZc0Pj95jaY aL2dj7venW2BjDGJdovE5n8= X-Google-Smtp-Source: AOwi7QDHVLw6KnmpW7PhyEW+wrhAgczhM0EBO3jc0vBb+J8K0WeVs9JepvGme3Sc8aCvxQ4tYaswZg== X-Received: by 10.13.223.10 with SMTP id i10mr2205656ywe.500.1507050525789; Tue, 03 Oct 2017 10:08:45 -0700 (PDT) Received: from ?IPv6:2620:10d:c0a3:20fb:7500:e7fb:4a6f:2254? ([2620:10d:c091:200::1:89d]) by smtp.googlemail.com with ESMTPSA id p193sm1130662ywp.14.2017.10.03.10.08.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 03 Oct 2017 10:08:45 -0700 (PDT) To: GCC Patches Cc: Jason Merrill , uweigand@de.ibm.com, Richard Biener , Jan Hubicka From: Nathan Sidwell Subject: [PATCH] C++ warning on vexing parse Message-ID: <2305655f-3433-a199-e49a-76bfc12986c4@acm.org> Date: Tue, 3 Oct 2017 13:08:43 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 [non-c++ people on CC, there's a reason ...] This patch adds a new warning, concerning unnecessary parentheses on a declaration. For instance: prettyprinter (pp); That's a declaration of a pp variable of type prettyprinter -- not a call of a prettyprinter function. The reason is that in C++, anything that is syntactically a declaration, is a declaration. This can lead to surprising unexpected code generation, and the documentation I added provides motivation: std::unique_lock (mymutex); That is NOT a lock of mymutex. It's a declaration of a variable called mymutex locking no mutex. If we're in a member function and 'mymutex' is a field, Wshadow-parm doesn't trigger either. This patch notes when a direct-declarator is parenthesized, and then warns about it in grokdeclarator. Obviously, parens are sometimes necessary -- when declaring pointers to arrays or functions, and we don't warn then. The simplest way to do that was during the parsing, not in grokdeclarator, where the cp_declarator object is constant. We'd either have to change that, or have some other local state. I added this to -Wparentheses (enabled by -Wall). It triggered in a few cases during bootstrap: 1) in toplev.c we have the above prettyprinter example. I think that's clearly poorly formatted code. 2) caller-save.c: insert_save has 'HARD_REG_SET (*to_save)', which also seems poorly formatted to me -- other cases of ptr-to-HRS don't do this. 3) A couple of places do: T (name // line break to avoid wrap [LONGEXPR][OTHERLONGEXPR]); The parens aid the editor's formatter. The patch removes the parens, but I can see there may be disagreement. I suppose we could permit parens at the outermost declarator for an array? Affects ira-int.h (target_ira_int::x_ira_reg_mode_hard_regset) reload.h (target_reload::x_regno_save_mode) 3) A set of typedef'd function types (NOT ptr-to-fn). The name being typedef'd is parenthesized and not an idiom I recall seeing before. AFAICT these are the only cases of typedefing a plain fn. We could, I suppose, ignore parens on a typedef'd fntype, but that seems a little random. Affects gengtype.c (frul_actionrout_t) lto-streamer.h (lto_get_section_data_f & to_free_section_data_f) tree-ssa-threadedge.c (pfn_simplify) If you've objections to the changes I've made in instances of #3 & #4 let me know. Jason, do you have concerns about the C++ patch itself? nathan 2017-10-03 Nathan Sidwell * caller-save.c (insert_save): Remove parens around TO_SAVE arg. * gengtype.c (frul_actionrout_t): Remove parens. * ira-int.h (target_ira_int::x_ira_reg_mode_hard_regset): Likewise. * lto-streamer.h (lto_get_section_data_f, lto_free_section_data_f): Remove parens. * reload.h (target_reload::x_regno_save_mode): Remove parens. * toplev.c (toplev::main): Remove parens on pp declaration. * tree-ssa-threadedge.c (pfn_simplify): Remove parens. * doc/invoke.texi (Wparentheses): Document C++ MVP behaviour. cp/ * cp-tree.h (struct cp_declarator): Add parenthesized field. * decl.c (grokdeclarator): Warn about unnecessary parens. * parser.c (make_declarator): Init parenthesized field. (make_call_declarator, make_array_declarator): Conditionally clear parenthesized field. (token_pair::open_loc): New accessor. (cp_parser_direct_declarator): Set parenthesized field. testsuite/ * g++.dg/warn/mvp.C: New. Index: caller-save.c =================================================================== --- caller-save.c (revision 253381) +++ caller-save.c (working copy) @@ -1265,7 +1265,7 @@ insert_restore (struct insn_chain *chain static int insert_save (struct insn_chain *chain, int before_p, int regno, - HARD_REG_SET (*to_save), machine_mode *save_mode) + HARD_REG_SET *to_save, machine_mode *save_mode) { int i; unsigned int k; Index: gengtype.c =================================================================== --- gengtype.c (revision 253381) +++ gengtype.c (working copy) @@ -1894,7 +1894,7 @@ get_file_gtfilename (const input_file *i */ /* Signature of actions in file rules. */ -typedef outf_p (frul_actionrout_t) (input_file*, char**, char**); +typedef outf_p frul_actionrout_t (input_file*, char**, char**); struct file_rule_st { Index: ira-int.h =================================================================== --- ira-int.h (revision 253381) +++ ira-int.h (working copy) @@ -801,8 +801,8 @@ struct target_ira_int { /* Map: hard regs X modes -> set of hard registers for storing value of given mode starting with given hard register. */ - HARD_REG_SET (x_ira_reg_mode_hard_regset - [FIRST_PSEUDO_REGISTER][NUM_MACHINE_MODES]); + HARD_REG_SET x_ira_reg_mode_hard_regset + [FIRST_PSEUDO_REGISTER][NUM_MACHINE_MODES]; /* Maximum cost of moving from a register in one class to a register in another class. Based on TARGET_REGISTER_MOVE_COST. */ Index: lto-streamer.h =================================================================== --- lto-streamer.h (revision 253381) +++ lto-streamer.h (working copy) @@ -278,19 +278,19 @@ lto_file_decl_data_num_ ## name ## s (st to be obtained. The third parameter is the name of the function and is only used when finding a function body; otherwise it is NULL. The fourth parameter is the length of the data returned. */ -typedef const char* (lto_get_section_data_f) (struct lto_file_decl_data *, - enum lto_section_type, - const char *, - size_t *); +typedef const char* lto_get_section_data_f (struct lto_file_decl_data *, + enum lto_section_type, + const char *, + size_t *); /* Return the data found from the above call. The first three parameters are the same as above. The fourth parameter is the data itself and the fifth is the length of the data. */ -typedef void (lto_free_section_data_f) (struct lto_file_decl_data *, - enum lto_section_type, - const char *, - const char *, - size_t); +typedef void lto_free_section_data_f (struct lto_file_decl_data *, + enum lto_section_type, + const char *, + const char *, + size_t); /* The location cache holds expanded locations for streamed in trees. This is done to reduce memory usage of libcpp linemap that strongly preffers Index: reload.h =================================================================== --- reload.h (revision 253381) +++ reload.h (working copy) @@ -174,9 +174,8 @@ struct target_reload { enough to save the entire contents of the register. When saving the register because it is live we first try to save in multi-register modes. If that is not possible the save is done one register at a time. */ - machine_mode (x_regno_save_mode - [FIRST_PSEUDO_REGISTER] - [MAX_MOVE_MAX / MIN_UNITS_PER_WORD + 1]); + machine_mode x_regno_save_mode[FIRST_PSEUDO_REGISTER] + [MAX_MOVE_MAX / MIN_UNITS_PER_WORD + 1]; /* Nonzero if an address (plus (reg frame_pointer) (reg ...)) is valid in the given mode. */ Index: toplev.c =================================================================== --- toplev.c (revision 253381) +++ toplev.c (working copy) @@ -2186,7 +2186,7 @@ toplev::main (int argc, char **argv) { gcc_assert (global_dc->edit_context_ptr); - pretty_printer (pp); + pretty_printer pp; pp_show_color (&pp) = pp_show_color (global_dc->printer); global_dc->edit_context_ptr->print_diff (&pp, true); pp_flush (&pp); Index: tree-ssa-threadedge.c =================================================================== --- tree-ssa-threadedge.c (revision 253381) +++ tree-ssa-threadedge.c (working copy) @@ -47,9 +47,8 @@ static int stmt_count; /* Array to record value-handles per SSA_NAME. */ vec ssa_name_values; -typedef tree (pfn_simplify) (gimple *, gimple *, - class avail_exprs_stack *, - basic_block); +typedef tree pfn_simplify (gimple *, gimple *, class avail_exprs_stack *, + basic_block); /* Set the value for the SSA name NAME to VALUE. */ Index: cp/cp-tree.h =================================================================== --- cp/cp-tree.h (revision 253381) +++ cp/cp-tree.h (working copy) @@ -5676,6 +5676,10 @@ struct cp_declarator { /* Whether we parsed an ellipsis (`...') just before the declarator, to indicate this is a parameter pack. */ BOOL_BITFIELD parameter_pack_p : 1; + /* If this declaration is parethesize, this the open-paren. It is + UNKNOWN_LOCATION when not parethesized. */ + location_t parenthesized; + location_t id_loc; /* Currently only set for cdk_id, cdk_decomp and cdk_function. */ /* GNU Attributes that apply to this declarator. If the declarator Index: cp/decl.c =================================================================== --- cp/decl.c (revision 253381) +++ cp/decl.c (working copy) @@ -10807,6 +10807,9 @@ grokdeclarator (const cp_declarator *dec attr_flags); } + if (declarator->parenthesized != UNKNOWN_LOCATION) + warning_at (declarator->parenthesized, OPT_Wparentheses, + "unnecessary parentheses in declaration of %qs", name); if (declarator->kind == cdk_id || declarator->kind == cdk_decomp) break; Index: cp/parser.c =================================================================== --- cp/parser.c (revision 253381) +++ cp/parser.c (working copy) @@ -1451,6 +1451,7 @@ make_declarator (cp_declarator_kind kind declarator = (cp_declarator *) alloc_declarator (sizeof (cp_declarator)); declarator->kind = kind; + declarator->parenthesized = UNKNOWN_LOCATION; declarator->attributes = NULL_TREE; declarator->std_attributes = NULL_TREE; declarator->declarator = NULL; @@ -1603,6 +1604,11 @@ make_call_declarator (cp_declarator *tar declarator = make_declarator (cdk_function); declarator->declarator = target; + /* Although parens are also superfluous if target is an array, + mixing arrays and functions is sufficiently confusing, let's not + check that. */ + if (target && target->kind != cdk_id && target->kind != cdk_function) + target->parenthesized = UNKNOWN_LOCATION; declarator->u.function.parameters = parms; declarator->u.function.qualifiers = cv_qualifiers; declarator->u.function.virt_specifiers = virt_specifiers; @@ -1633,6 +1639,11 @@ make_array_declarator (cp_declarator *el declarator = make_declarator (cdk_array); declarator->declarator = element; + /* Although parens are also superfluous if target is a function, + mixing arrays and functions is sufficiently confusing, let's not + check that. */ + if (element && element->kind != cdk_id && element->kind != cdk_array) + element->parenthesized = UNKNOWN_LOCATION; declarator->u.array.bounds = bounds; if (element) { @@ -4533,6 +4544,11 @@ class token_pair traits_t::required_token_open); } + location_t open_loc () const + { + return m_open_loc; + } + /* Consume the next token from PARSER, recording its location as that of the opening token within the pair. */ @@ -19986,6 +20002,8 @@ cp_parser_direct_declarator (cp_parser* = cp_parser_declarator (parser, dcl_kind, ctor_dtor_or_conv_p, /*parenthesized_p=*/NULL, member_p, friend_p); + if (declarator) + declarator->parenthesized = parens.open_loc (); parser->in_type_id_in_expr_p = saved_in_type_id_in_expr_p; first = false; /* Expect a `)'. */ Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 253381) +++ doc/invoke.texi (working copy) @@ -4579,6 +4579,16 @@ in the @code{?}: operator is a boolean e always 1. Often programmers expect it to be a value computed inside the conditional expression instead. +For C++ this also warns on unnecessary parentheses in declarations, which +can indicate an attempt at a function call instead of a declaration: +@smallexample +@{ + // Declares a local variable called mymutex. + std::unique_lock (mymutex); + // User meant std::unique_lock lock (mymutex); +@} +@end smallexample + This warning is enabled by @option{-Wall}. @item -Wsequence-point Index: testsuite/g++.dg/warn/mvp.C =================================================================== --- testsuite/g++.dg/warn/mvp.C (revision 0) +++ testsuite/g++.dg/warn/mvp.C (working copy) @@ -0,0 +1,68 @@ +// { dg-additional-options -Wparentheses } + +// Most Vexing Parse warnings +// in C++ anythig that syntactically looks like a decl IS a decl, this +// can lead to confused users, but worse silent unexpectedly unsafe +// code generation. + +int (a); // { dg-warning "" } +int (*b); // { dg-warning "" } +extern int (&c); // { dg-warning "" } + +namespace fns +{ + int (*a) (); + int (b) (); // { dg-warning "" } + int (*c ()) (); + int (d ()); // { dg-warning "" } + int (e) (int); // { dg-warning "" } + int g (int (a)); // { dg-warning "in declaration of 'a'" } + int (h) (int (a)); // { dg-warning "in declaration of 'a'" } + // { dg-warning "in declaration of 'h'" "" { target *-*-* } .-1 } +} + +namespace arys +{ + int (*a)[1]; + int (b)[1]; // { dg-warning "" } + int (*c[1])[1]; + int (d[1]); // { dg-warning "" } + int (e[1])[1]; // { dg-warning "" } +} + +namespace complex +{ + int (*a())[1]; + int (*b[1])(); + int ((*c())[1]); // { dg-warning "" } + int ((*d[1])()); // { dg-warning "" } +} + +namespace motivation +{ + typedef int shared_mutex; // for exposition + struct locker + { + locker (); + locker (int &r); + ~locker (); + }; + class protected_state + { + shared_mutex mutex; // not a real mutex type + int state; + + public: + void not_thread_safe () + { + locker (mutex); // { dg-warning "" } + state++; // oops + } + + void thread_safe () + { + locker lock (mutex); + state++; // ok; + } + }; +}