On Tue, Oct 3, 2017 at 1:08 PM, Nathan Sidwell <nat...@acm.org> wrote: > [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.
Definitely. > 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. Probably autopilot from someone used to parenthesizing declarators for pointer-to-function or pointer-to-array types. > 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) We could also check whether the declarator spans lines to detect this case. > 4) 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) Hmm, I don't think we want to diagnose these; if a parameter-list follows the parenthesized declarator, it isn't ambiguous. 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 > Jason, do you have concerns about the C++ patch itself? Nope. Jason