[cfe-dev] PR5478 Patch - unexpected function pointer in argument diagnostic improvement

[Samuel Harrington]

Ok, here's a new version of the patch. Thanks for your review! Responses below.

On Mon, Mar 29, 2010 at 9:22 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> +  // Diagnose missing parens after function name argument
> +  if (FromTy->isFunctionType() && !ToTy->isFunctionType()
> +    && FromTy->getAs<FunctionType>()->getResultType() == ToTy
>
> The test comparing getResultType() to ToTy actually checks that the types were spelled exactly the same way, e.g., 'int' would not be the same as a typedef of 'int'. To determine whether the types are semantically equivalent, use
>
>        Context.hasSameType(FromTy->getAs<FunctionType>()->getResultType(), ToTy)
>
> Or, you could go a step further and use InitializationSequence to determine whether the result type of the function can be passed to an argument of the type ToTy. That would allow this special diagnostic to work when there's some conversion involved (e.g., the function might return a pointer to a derived class of A).
>

I went with the last method, but I'm not entirely sure I used it correctly.

> Three more little comments about this part:
>
>  1) The result type of a function can be a reference type, but an argument cannot have reference type. Call getNonReferenceType() on the return value of getResultType() to look through a reference type when it's there.
>

Done.

>  2) The suggested code-modification hint only works if the function takes 0 parameters. Your code should check this before using the special diagnostic.
>

Done.

>  3) Did you want to deal with the case where FromTy is a pointer-to-function (e.g., a variable of pointer-to-function type)?
>

I'm not sure what would be necessary for this case, but if it does not
complicate things too much, sure. I think it would require another
diagnostic though, so perhaps it should be in a follow-up patch.

> +    S.Diag(FromExpr->getExprLoc(), diag::note_ovl_candidate_bad_conv_funcptr)
> +      << CodeModificationHint::CreateInsertion(S.PP.getLocForEndOfToken(FromExpr->getLocEnd()), "()");
>
> This is going to emit a second note, after we've already said "bad conversion". That could get noisy if there are several overload candidates with the same problem. How about creating a variant of note_ovl_candidate_bad_conv that you can use instead of note_ovl_candidate_bad_conv, which says something about the missing '()' in the same note? We do this for, e.g., arguments of incomplete type and it's rather nice (see note_ovl_candidate_bad_conv_incomplete).
>

Ok, done. Please note that the diagnostic for this case now points to
the call instead of the declaration of the function, so the code
modification hint can be displayed. For this case, I think this is
more understandable.

>        - Doug

Comments appreciated!

Thanks,
Sam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: func_as_arg_v2.patch
Type: application/octet-stream
Size: 5795 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100330/8dac28d6/attachment.obj>