<div class="gmail_quote">On Wed, Jul 18, 2012 at 7:26 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">
<div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><div>We should still be warning on this line, because it could either be declaring d to be a pointer to a function which returns T, or to be a pointer to T initialized by int(p). The place to handle this is probably GetFullTypeForDeclarator in SemaType.cpp. I don't know if you can move all of this checking to there, but if you can, that would be ideal. If not, splitting it up is OK.</div>



<div><br></div></blockquote><div><br></div></div><div>I moved everything to SemaType.cpp but I have a number of questions:</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<div></div><div>Please factor the D.isAmbiguous() && !T->isVoidType() tests into an outer 'if'.</div></blockquote><div><br></div></div><div>I couldn't do this inside SemaDecl.cpp as the call to D.getTypeObject(0) crashed when I moved it outside the 'if'. Now that it's in SemaType.cpp, it seems to be working even outside the 'if''. Hope this isn't by chance, but tests show no problems.</div>
</div></blockquote><div><br></div><div>This isn't by chance; we're processing one of the type objects, so there must be at least one. However, you should just use FTI there. Using that, rather than just looking at getTypeObject(0), will fix a couple of bugs; we should warn on these cases:</div>
<div><br></div><div>typedef int *IntPtr;</div><div>void f() {</div><div>  int *p();</div><div>  int *q(IntPtr());</div><div>}</div><div><br></div><div>Without your patch, we only warn on the second case. With your patch, we don't warn on either case. You should also remove the check that D.getNumTypeObjects() == 1; that was being used as part of the check for whether we had an ambiguity, and was overly-conservative.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div> </div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>It would be great to say "as a function declaration" here (but still use the "as a function declarator" in the pointer-to-function case).</div>


<div><br></div></blockquote><div><br></div></div><div>I added the message text, but I don't know how to differentiate between the two cases. I can see that D.getNumTypeObjects() returns 3 for the 'pointer to function' case, but I don't really understand what this function does. Same goes for D.getNumTypeObjects() == 1 condition.</div>
</div></blockquote><div><br></div><div>The type objects work like this: suppose you have this declaration:</div><div><br></div><div>int *(*(f))(int());</div><div><br></div><div>We have the following type objects ("chunks") in the Declarator for f (starting at 'f' and working outwards):</div>
<div><br></div><div>[0]: DeclaratorChunk::Paren for parens around f</div><div>[1]: DeclaratorChunk::Pointer</div><div>[2]: DeclaratorChunk::Paren</div><div>[3]: DeclaratorChunk::Function, with argument type int()</div><div>
[4]: DeclaratorChunk::Pointer</div><div><br></div><div>You should use the 'function declaration' diagnostic if there are only DeclaratorChunk::Paren objects before the Function chunk. You can test that with D.isFunctionDeclarator().</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>Checking for FunctionTemplate would be another issue. These warnings don't fire at file scope, I'm guessing that this is because function declarations are much more likely here than global variables?</div>
</div></blockquote><div><br></div><div>Yes, exactly.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>It seems that template declarations are not allowed at block scope so this check is not really needed. I might be wrong.</div>
</div></blockquote><div><br></div><div>Yes, dropping that check is fine.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>
Function return type used to be obtained from TypeSourceInfo but this parameter is 0 inside GetFullTypeForDeclaration. I saw that the rest of code uses declSpecType for checks about the return type so this is what I used too.</div>


<div><br></div><div>One of the diagnostics used NewFD variable (FunctionDecl *) in the message output, but I used the identifier that was used to do a lookup. I'm guessing that this works the same and only outputs the function name. The test case for this passes with my modification.</div>
</div></blockquote><div><br></div><div>Yes, this is fine.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>ActOnFuctionDeclaration had a 'Scope *' parameter but GetFullTypeForDeclaration doesn't.  I used S.getCurScope() to get the scope, but I'm not sure that this is entirely correct because I saw this in Sema::HandleDeclarator:</div>


<div><br></div></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div class="gmail_quote"><div><div>  // The scope passed in may not be a decl scope.  Zip up the scope tree until</div></div></div><div class="gmail_quote">


<div><div>  // we find one that is.</div></div></div><div class="gmail_quote"><div><div>  while ((S->getFlags() & Scope::DeclScope) == 0 ||</div></div></div><div class="gmail_quote"><div><div>         (S->getFlags() & Scope::TemplateParamScope) != 0)</div>


</div></div><div class="gmail_quote"><div><div>    S = S->getParent();</div></div></div></blockquote><div class="gmail_quote"><div><br></div><div>Comments are breaking the 80 width policy by one character and I have no idea what to do about them. I'm also not sure if we need the 'We don't catch this case earlier, since there is no ambiguity here.' comment.</div>
</div></blockquote><div><br></div><div>Yes, we don't need that; it's a relic of TryParseParameterDeclarationClause returning TPResult::True for empty parens. Please wrap the comments to 80 columns, even if you're left with just one word on a line.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>This was a bit long but since I moved a lot of code it might be easier for you to check every change this way. All test are passing except the one with function pointer that now expects different wording.</div>
</div></blockquote><div><br></div><div>Just two other things: there are some undesirable whitespace changes in the patch (the last two changes in ParseDecl.cpp and the middle hunk in DeclSpec.cpp); please revert those. And you have a typo "emmit" (should be "emit") in fixit-vexing-parse.cpp.</div>
<div><br></div><div>Thanks again!</div></div>