[cfe-commits] Note and FixIt for additional case of vexing parse

[Richard Smith]

On Sat, Jul 14, 2012 at 5:00 AM, Nikola Smiljanic <popizdeh at gmail.com>wrote:

> Thanks for your insightful comments Richard, I've been really busy but
> I'll hopefully find some time to finish this. Here's a new patch just
> to check that I'm on track:
>
> I moved the warning completely to Sema, I'm not sure if this is the
> right approach?
>

We can't perform the T->isVoidType() check accurately until we get to Sema,
so this seems appropriate.


> I don't know how to differentiate between int foo(int) which is
> obviously a function declaration and int foo(int()) which could be a
> variable declaration. What I'm looking for is a check whether the
> function declaration is ambiguous?


Maybe the parser could store a flag in the DeclSpec to indicate that we hit
the function / direct-init ambiguity? That would also allow you to simplify
the code in Sema a bit, for both the case where the function has arguments
and the case where it does not: you could drop the checks for variadic
functions, trailing return types and exception specifications, and the Sema
code would become robust against future grammar changes (for this to work
for the no-args case, you'll need to change
Parser::TryParseParameterDeclarationClause to return TPResult::Ambiguous()
rather than TPResult::True() for an empty parameter-declaration-clause).


> > Your FixIt will cause these declarations to use vector's initializer_list
> > constructor, which is presumably not what the author of the code
> intended.
> > We can't detect these cases from within the parser, so this check should
> > either always use the C++98 note, or (preferably, but more work) be
> moved to
> > Sema (alongside the "int n()" vexing parse diagnostic code) and use the
> > braces note only if there is no initializer_list constructor.
>
> I'm starting to think that this note doesn't make sense even in that
> case, because what if somebody suppresses the warning and later on
> adds a constructor with an initializer list? Adding a pair of braces
> could change the meaning of the code, and we don't want this, right?


I'm not too concerned about the possibility of someone applying a fixit
hint and later making a code change elsewhere which causes the fixed code
to break, unless the fixit inherently makes the code more prone to break
(and I don't think that's particularly likely to apply in this case).

It's up to you: if you want to issue the braces fixit, it should be
correct. If you don't want to go to the effort of working out when it'd be
correct, just use the parens fixit, but please leave a FIXME in the code
suggesting that we consider adding such a fixit later.


> > This change will also produce an unhelpful note for this case (in C++98
> > mode)...
> >
> >   if (int n(int())) {
> >
> > ... because a direct-initializer isn't actually permitted on this
> > declaration, but we perform a tentative parse anyway in order to produce
> the
> > 'variable declaration in condition cannot have a parenthesized
> initializer'
> > diagnostic. For now, I suggest you just set warnIfAmbiguous to false in
> > ParseDirectDeclarator when we're in a Declarator::ConditionContext.
>
> What would be the right way to check for this case in the Sema? I can
> do D.Context != ConditionContext inside my if statement?


Yes, that would be reasonable.


> > This fixit isn't correct if multiple parameters are declared: you're
> > suggesting adding a single set of parentheses around all the parameters;
> > instead, you should suggest adding them around just one of the
> parameters.
>
> This should be correct now, but I have a question regarding fixit
> insertion. The fixit is inserted before the location it points to,
> just like iterators in stl? My fixit for "int foo(int());" points to
> "i" of the parameter, and to the last closing paren (the one before
> the semicolon). But I could achieve the same result without calling
> PP.getLocForEndOfToken at all, but the insertion points wouldn't be
> 100% accurate. The first one would point to foo's open paren and the
> second one would point to first parameter's closing paren. I'm not
> sure what the convention here is?
>

An insertion fixit adds characters immediately before the character
indicated by the SourceLocation. In your case, we want to insert the '('
immediately before the first token in the parameter declaration, and the
')' immediately after the last token in it.

For these locations, could you use the start and end of
ArgInfo[0].Param->getSourceRange()? (You'd need to use getLocForEndOfToken
on the end). I think your current approach can put the ')' in the wrong
place in some pathological cases, like:

  float arr[5];
  int x(int(arr[0]));
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120714/c3248a87/attachment.html>