[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<std::mutex> (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
--
Nathan Sidwell
2017-10-03 Nathan Sidwell <nat...@acm.org>
* 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<tree> 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<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: 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;
+ }
+ };
+}
