On Thu, Oct 29, 2020 at 11:17:37AM -0400, Jason Merrill via Gcc-patches wrote:
> On 10/28/20 7:40 PM, Marek Polacek wrote:
> > On Wed, Oct 28, 2020 at 03:09:08PM -0400, Jason Merrill wrote:
> > > On 10/28/20 1:58 PM, Marek Polacek wrote:
> > > > On Wed, Oct 28, 2020 at 01:26:53AM -0400, Jason Merrill via Gcc-patches
> > > > wrote:
> > > > > On 10/24/20 7:40 PM, Marek Polacek wrote:
> > > > > > On Fri, Oct 23, 2020 at 09:33:38PM -0400, Jason Merrill via
> > > > > > Gcc-patches wrote:
> > > > > > > On 10/23/20 3:01 PM, Marek Polacek wrote:
> > > > > > > > This patch implements the -Wvexing-parse warning to warn about
> > > > > > > > the
> > > > > > > > sneaky most vexing parse rule in C++: the cases when a
> > > > > > > > declaration
> > > > > > > > looks like a variable definition, but the C++ language requires
> > > > > > > > it
> > > > > > > > to be interpreted as a function declaration. This warning is
> > > > > > > > on by
> > > > > > > > default (like clang++). From the docs:
> > > > > > > >
> > > > > > > > void f(double a) {
> > > > > > > > int i(); // extern int i (void);
> > > > > > > > int n(int(a)); // extern int n (int);
> > > > > > > > }
> > > > > > > >
> > > > > > > > Another example:
> > > > > > > >
> > > > > > > > struct S { S(int); };
> > > > > > > > void f(double a) {
> > > > > > > > S x(int(a)); // extern struct S x (int);
> > > > > > > > S y(int()); // extern struct S y (int (*) (void));
> > > > > > > > S z(); // extern struct S z (void);
> > > > > > > > }
> > > > > > > >
> > > > > > > > You can find more on this in [dcl.ambig.res].
> > > > > > > >
> > > > > > > > I spent a fair amount of time on fix-it hints so that GCC can
> > > > > > > > recommend
> > > > > > > > various ways to resolve such an ambiguity. Sometimes that's
> > > > > > > > tricky.
> > > > > > > > E.g., suggesting default-initialization when the class doesn't
> > > > > > > > have
> > > > > > > > a default constructor would not be optimal. Suggesting {}-init
> > > > > > > > is also
> > > > > > > > not trivial because it can use an initializer-list constructor
> > > > > > > > if no
> > > > > > > > default constructor is available (which ()-init wouldn't do).
> > > > > > > > And of
> > > > > > > > course, pre-C++11, we shouldn't be recommending {}-init at all.
> > > > > > >
> > > > > > > What do you think of, instead of passing the type down into the
> > > > > > > declarator
> > > > > > > parse, adding the paren locations to cp_declarator::function and
> > > > > > > giving the
> > > > > > > diagnostic from cp_parser_init_declarator instead?
> > > > >
> > > > > Oops, now I see there's already cp_declarator::parenthesized; might
> > > > > as well
> > > > > reuse that. And maybe change it to a range, while we're at it.
> > > >
> > > > I'm afraid I can't reuse it because grokdeclarator uses it to warn about
> > > > "unnecessary parentheses in declaration". So when we have:
> > > >
> > > > int (x());
> > > >
> > > > declarator->parenthesized points to the outer parens (if any), whereas
> > > > declarator->u.function.parens_loc should point to the inner ones. We
> > > > also
> > > > have declarator->id_loc but I think we should only use it for
> > > > declarator-ids.
> > >
> > > Makes sense.
> > >
> > > > (We should still adjust ->parenthesized to be a range to generate a
> > > > better
> > > > diagnostic; I shall send a patch soon.)
> > > >
> > > > > Hmm, I wonder why we have the parenthesized_p parameter to some of
> > > > > these
> > > > > functions, since we can look at the declarator to find that
> > > > > information...
> > > >
> > > > That would be a nice cleanup.
> > > >
> > > > > > Interesting idea. I suppose it's better, and makes the
> > > > > > implementation
> > > > > > more localized. The approach here is that if the
> > > > > > .function.parens_loc
> > > > > > is UNKNOWN_LOCATION, we've not seen a vexing parse.
> > > > >
> > > > > I'd rather always set the parens location, and then analyze the
> > > > > cp_declarator in warn_about_ambiguous_parse to see if it's a vexing
> > > > > parse;
> > > > > we should have all the information we need.
> > > >
> > > > I could always set .parens_loc, but then I'd still need another flag
> > > > telling
> > > > me whether we had an ambiguity. Otherwise I don't know how I would tell
> > > > apart e.g. "int f()" (warn) v. "int f(void)" (don't warn), etc.
> > >
> > > Ah, I was thinking that we still had the parameter declarators, but now I
> > > see that cp_parser_parameter_declaration_list groks them and returns a
> > > TREE_LIST. We could set a TREE_LANG_FLAG on each TREE_LIST if its
> > > parameter
> > > declarator was parenthesized?
> >
> > I think so, looks like we have a bunch of free TREE_LANG_FLAG slots on
> > a TREE_LIST. But cp_parser_parameter_declaration_clause can return
> > a void_list_node, so I assume I'd have to copy_node it before setting
> > some new flag in it. Do you think that'd be fine?
>
> There's no declarator in a void_list_node, so we shouldn't need to set a
> "declarator is parenthesized" flag on it.
I guess I'm still not clear on how I would distinguish between
int f() and int f(void). When I look at the cdk_function declarator,
all I can see is the .parameters TREE_LIST, which for both cases will
be the same void_list_node, but we should only warn for the former.
What am I missing?
Marek
