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

[Richard Smith]

On Wed, Jul 18, 2012 at 7:26 AM, Nikola Smiljanic <popizdeh at gmail.com>wrote:

>
>> 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.
>>
>>
> I moved everything to SemaType.cpp but I have a number of questions:
>
>
>> Please factor the D.isAmbiguous() && !T->isVoidType() tests into an outer
>> 'if'.
>>
>
> 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.
>

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:

typedef int *IntPtr;
void f() {
  int *p();
  int *q(IntPtr());
}

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.


>
>
>>
>> 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).
>>
>>
> 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.
>

The type objects work like this: suppose you have this declaration:

int *(*(f))(int());

We have the following type objects ("chunks") in the Declarator for f
(starting at 'f' and working outwards):

[0]: DeclaratorChunk::Paren for parens around f
[1]: DeclaratorChunk::Pointer
[2]: DeclaratorChunk::Paren
[3]: DeclaratorChunk::Function, with argument type int()
[4]: DeclaratorChunk::Pointer

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().


> 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?
>

Yes, exactly.


> It seems that template declarations are not allowed at block scope so this
> check is not really needed. I might be wrong.
>

Yes, dropping that check is fine.


> 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.
>
> 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.
>

Yes, this is fine.


> 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:
>
>   // The scope passed in may not be a decl scope.  Zip up the scope tree
> until
>   // we find one that is.
>   while ((S->getFlags() & Scope::DeclScope) == 0 ||
>          (S->getFlags() & Scope::TemplateParamScope) != 0)
>     S = S->getParent();
>
>
> 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.
>

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.


> 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.
>

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.

Thanks again!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120718/28a55e5b/attachment.html>