From 807e3be2b82b3e15572ac6b0128e9b7dd306fdc4 Mon Sep 17 00:00:00 2001 From: Nathan Sidwell Date: Thu, 5 Oct 2017 13:27:37 +0000 Subject: [PATCH] Re: [PATCH] C++ warning on vexing parse https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00249.html gcc/cp/ Warn on MVP declarations * cp-tree.h (struct cp_declarator): Add parenthesized field. * decl.c (grokdeclarator): Warn about unnecessary parens. * parser.c (make_declarator): Init parenthesized field. (cp_parser_direct_declarator): Set parenthesized field. gcc/ * doc/invoke.texi (Wparentheses): Document C++ MVP behaviour. gcc/testsuite/ * g++.dg/warn/mvp.C: New. From-SVN: r253446 --- gcc/ChangeLog | 4 ++ gcc/cp/ChangeLog | 6 +++ gcc/cp/cp-tree.h | 4 ++ gcc/cp/decl.c | 7 +++ gcc/cp/parser.c | 23 ++++++++++ gcc/doc/invoke.texi | 11 +++++ gcc/testsuite/ChangeLog | 4 ++ gcc/testsuite/g++.dg/warn/mvp.C | 78 +++++++++++++++++++++++++++++++++ 8 files changed, 137 insertions(+) create mode 100644 gcc/testsuite/g++.dg/warn/mvp.C diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 27d17820575..8b735d7f5a5 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,7 @@ +2017-10-05 Nathan Sidwell + + * doc/invoke.texi (Wparentheses): Document C++ MVP behaviour. + 2017-10-05 Tamar Christina * config/arm/arm.c (arm_test_fpu_data): New. diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog index 75778465200..b5ae7451513 100644 --- a/gcc/cp/ChangeLog +++ b/gcc/cp/ChangeLog @@ -1,5 +1,11 @@ 2017-10-05 Nathan Sidwell + Warn on MVP declarations + * cp-tree.h (struct cp_declarator): Add parenthesized field. + * decl.c (grokdeclarator): Warn about unnecessary parens. + * parser.c (make_declarator): Init parenthesized field. + (cp_parser_direct_declarator): Set parenthesized field. + Kill IDENTIFIER_GLOBAL_VALUE, SET_IDENTIFIER_GLOBAL_VALUE * cp-tree.h (IDENTIFIER_GLOBAL_VALUE, SET_IDENTIFIER_GLOBAL_VALUE): Delete. diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 5f9874ba10b..8445bfae6e5 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -5671,6 +5671,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 declarator is parenthesized, this the open-paren. It is + UNKNOWN_LOCATION when not parenthesized. */ + 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 diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 2b4ed0131e0..7c68f68b9bd 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -10807,6 +10807,13 @@ grokdeclarator (const cp_declarator *declarator, attr_flags); } + /* We don't want to warn in parmeter context because we don't + yet know if the parse will succeed, and this might turn out + to be a constructor call. */ + if (decl_context != PARM + && 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; diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 28bc8e4d42a..3b0ee3934f2 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -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; @@ -19808,6 +19809,7 @@ cp_parser_direct_declarator (cp_parser* parser, bool saved_in_declarator_p = parser->in_declarator_p; bool first = true; tree pushed_scope = NULL_TREE; + cp_token *open_paren = NULL, *close_paren = NULL; while (true) { @@ -19858,6 +19860,8 @@ cp_parser_direct_declarator (cp_parser* parser, tree params; bool is_declarator = false; + open_paren = NULL; + /* In a member-declarator, the only valid interpretation of a parenthesis is the start of a parameter-declaration-clause. (It is invalid to @@ -19979,6 +19983,7 @@ cp_parser_direct_declarator (cp_parser* parser, parser->default_arg_ok_p = saved_default_arg_ok_p; parser->in_declarator_p = saved_in_declarator_p; + open_paren = token; /* Consume the `('. */ matching_parens parens; parens.consume_open (parser); @@ -19992,6 +19997,7 @@ cp_parser_direct_declarator (cp_parser* parser, parser->in_type_id_in_expr_p = saved_in_type_id_in_expr_p; first = false; /* Expect a `)'. */ + close_paren = cp_lexer_peek_token (parser->lexer); if (!parens.require_close (parser)) declarator = cp_error_declarator; if (declarator == cp_error_declarator) @@ -20013,6 +20019,7 @@ cp_parser_direct_declarator (cp_parser* parser, if (ctor_dtor_or_conv_p) *ctor_dtor_or_conv_p = 0; + open_paren = NULL; first = false; parser->default_arg_ok_p = false; parser->in_declarator_p = true; @@ -20308,6 +20315,22 @@ cp_parser_direct_declarator (cp_parser* parser, point. That's an error; the declarator is not optional. */ if (!declarator) cp_parser_error (parser, "expected declarator"); + else if (open_paren) + { + /* Record overly parenthesized declarator so we can give a + diagnostic about confusing decl/expr disambiguation. */ + if (declarator->kind == cdk_array) + { + /* If the open and close parens are on different lines, this + is probably a formatting thing, so ignore. */ + expanded_location open = expand_location (open_paren->location); + expanded_location close = expand_location (close_paren->location); + if (open.line != close.line || open.file != close.file) + open_paren = NULL; + } + if (open_paren) + declarator->parenthesized = open_paren->location; + } /* If we entered a scope, we must exit it now. */ if (pushed_scope) diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index f862b7f8c99..84cc43a7355 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -4579,6 +4579,17 @@ in the @code{?}: operator is a boolean expression, the omitted value is always 1. Often programmers expect it to be a value computed inside the conditional expression instead. +For C++ this also warns for some cases of 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 diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 829d999aa34..8b462b40fa0 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2017-10-05 Nathan Sidwell + + * g++.dg/warn/mvp.C: New. + 2017-10-05 Jakub Jelinek * gcc.dg/gomp/pr82374.c (SIZE): Change from 1G to 1M to make it ilp32 diff --git a/gcc/testsuite/g++.dg/warn/mvp.C b/gcc/testsuite/g++.dg/warn/mvp.C new file mode 100644 index 00000000000..9fbe2272233 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/mvp.C @@ -0,0 +1,78 @@ +// { 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 "" } + +int h1 = 0, h2 = 0; +struct H { H(...);}; + +namespace fns +{ + int (*a) (); + int (b) (); + int (*c ()) (); + int (d1 ()); // { dg-warning "" } + int (d2 // { dg-warning "" } + ()); + int (e) (int); + int g (int (a)); // No warning because ... + H h (int (h1), int (h2), 3); // ... not a function decl. +} + +namespace arys +{ + int (*a)[1]; + int (b)[1]; + int (*c[1])[1]; + int (d1[1]); // { dg-warning "" } + int (d2 + [1]); + int (e[1])[1]; +} + +namespace complex +{ + int (*a())[1]; + int (*b[1])(); + int ((*c1())[1]); // { dg-warning "" } + int ((*c2()) + [1]); + int ((*d1[1])()); // { dg-warning "" } + int ((*d2[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; + } + }; +} -- 2.30.2