Re: [PATCH] C++ warning on vexing parse
authorNathan Sidwell <nathan@acm.org>
Thu, 5 Oct 2017 13:27:37 +0000 (13:27 +0000)
committerNathan Sidwell <nathan@gcc.gnu.org>
Thu, 5 Oct 2017 13:27:37 +0000 (13:27 +0000)
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
gcc/cp/ChangeLog
gcc/cp/cp-tree.h
gcc/cp/decl.c
gcc/cp/parser.c
gcc/doc/invoke.texi
gcc/testsuite/ChangeLog
gcc/testsuite/g++.dg/warn/mvp.C [new file with mode: 0644]

index 27d1782..8b735d7 100644 (file)
@@ -1,3 +1,7 @@
+2017-10-05  Nathan Sidwell  <nathan@acm.org>
+
+       * doc/invoke.texi (Wparentheses): Document C++ MVP behaviour.
+
 2017-10-05  Tamar Christina  <tamar.christina@arm.com>
 
        * config/arm/arm.c (arm_test_fpu_data): New.
index 7577846..b5ae745 100644 (file)
@@ -1,5 +1,11 @@
 2017-10-05  Nathan Sidwell  <nathan@acm.org>
 
+       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.
index 5f9874b..8445bfa 100644 (file)
@@ -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 2b4ed01..7c68f68 100644 (file)
@@ -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;
 
index 28bc8e4..3b0ee39 100644 (file)
@@ -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)
index f862b7f..84cc43a 100644 (file)
@@ -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<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 829d999..8b462b4 100644 (file)
@@ -1,3 +1,7 @@
+2017-10-05  Nathan Sidwell  <nathan@acm.org>
+
+       * g++.dg/warn/mvp.C: New.
+
 2017-10-05  Jakub Jelinek  <jakub@redhat.com>
 
        * 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 (file)
index 0000000..9fbe227
--- /dev/null
@@ -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;
+    }
+  };
+}