<div class="gmail_quote">On Sat, Jul 14, 2012 at 5:00 AM, Nikola Smiljanic <span dir="ltr"><<a href="mailto:popizdeh@gmail.com" target="_blank">popizdeh@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks for your insightful comments Richard, I've been really busy but<br>
I'll hopefully find some time to finish this. Here's a new patch just<br>
to check that I'm on track:<br>
<br>
I moved the warning completely to Sema, I'm not sure if this is the<br>
right approach?<br></blockquote><div><br></div><div>We can't perform the T->isVoidType() check accurately until we get to Sema, so this seems appropriate.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

I don't know how to differentiate between int foo(int) which is<br>
obviously a function declaration and int foo(int()) which could be a<br>
variable declaration. What I'm looking for is a check whether the<br>
function declaration is ambiguous?</blockquote><div><br></div><div>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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> Your FixIt will cause these declarations to use vector's initializer_list<br>
> constructor, which is presumably not what the author of the code intended.<br>
> We can't detect these cases from within the parser, so this check should<br>
> either always use the C++98 note, or (preferably, but more work) be moved to<br>
> Sema (alongside the "int n()" vexing parse diagnostic code) and use the<br>
> braces note only if there is no initializer_list constructor.<br>
<br>
</div>I'm starting to think that this note doesn't make sense even in that<br>
case, because what if somebody suppresses the warning and later on<br>
adds a constructor with an initializer list? Adding a pair of braces<br>
could change the meaning of the code, and we don't want this, right?</blockquote><div><br></div><div>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).</div>
<div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">> This change will also produce an unhelpful note for this case (in C++98</div><div class="im">

> mode)...<br>
><br>
>   if (int n(int())) {<br>
><br>
> ... because a direct-initializer isn't actually permitted on this<br>
> declaration, but we perform a tentative parse anyway in order to produce the<br>
> 'variable declaration in condition cannot have a parenthesized initializer'<br>
> diagnostic. For now, I suggest you just set warnIfAmbiguous to false in<br>
> ParseDirectDeclarator when we're in a Declarator::ConditionContext.<br>
<br>
</div>What would be the right way to check for this case in the Sema? I can<br>
do D.Context != ConditionContext inside my if statement?</blockquote><div><br></div><div>Yes, that would be reasonable.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> This fixit isn't correct if multiple parameters are declared: you're<br>
> suggesting adding a single set of parentheses around all the parameters;<br>
> instead, you should suggest adding them around just one of the parameters.<br>
<br>
</div>This should be correct now, but I have a question regarding fixit<br>
insertion. The fixit is inserted before the location it points to,<br>
just like iterators in stl? My fixit for "int foo(int());" points to<br>
"i" of the parameter, and to the last closing paren (the one before<br>
the semicolon). But I could achieve the same result without calling<br>
PP.getLocForEndOfToken at all, but the insertion points wouldn't be<br>
100% accurate. The first one would point to foo's open paren and the<br>
second one would point to first parameter's closing paren. I'm not<br>
sure what the convention here is?<br>
</blockquote></div><br><div>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.</div>
<div><br></div><div>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:</div>
<div><br></div><div>  float arr[5];</div><div>  int x(int(arr[0]));</div>