On 10/04/2017 10:44 AM, Jason Merrill wrote:
Hmm, I don't think we want to diagnose these; if a parameter-list
follows the parenthesized declarator, it isn't ambiguous.
True. I went with that approach
3 and 4 seem like false positives. The problematic cases are all ones
where the parenthesized name is at the end of the declarator; if the
declarator continues after the name, either within or without the
parens, this
I went with this approach. For an array declarator of the form:
T (ary...[X]);
we only warn when the '(' and ')' are on the same line.
Thus no triggers on GCC's source base (other than the earlier two I
already fixed).
Applying the attached
nathan
--
Nathan Sidwell
2017-10-05 Nathan Sidwell <nat...@acm.org>
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.
Index: gcc/cp/cp-tree.h
===================================================================
--- gcc/cp/cp-tree.h (revision 253445)
+++ gcc/cp/cp-tree.h (working copy)
@@ -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
Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c (revision 253445)
+++ gcc/cp/decl.c (working copy)
@@ -10807,6 +10807,13 @@ grokdeclarator (const cp_declarator *dec
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;
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c (revision 253445)
+++ gcc/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;
@@ -19808,6 +19809,7 @@ cp_parser_direct_declarator (cp_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*
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->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->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*
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*
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)
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi (revision 253445)
+++ gcc/doc/invoke.texi (working copy)
@@ -4579,6 +4579,17 @@ 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 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<std::mutex> (mymutex);
+ // User meant std::unique_lock<std::mutex> lock (mymutex);
+@}
+@end smallexample
+
This warning is enabled by @option{-Wall}.
@item -Wsequence-point
Index: gcc/testsuite/g++.dg/warn/mvp.C
===================================================================
--- gcc/testsuite/g++.dg/warn/mvp.C (revision 0)
+++ gcc/testsuite/g++.dg/warn/mvp.C (working copy)
@@ -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;
+ }
+ };
+}
