PR5478 Patch - unexpected function pointer in argument diagnostic improvement

Hello,

I've been a long-time lurker, and decided I would try my hand at
working on a simple patch. Attached is a patch to improve the
diagnostic in C++ for the following case, from PR5478:

struct A;
A* inner(....);
void outer(A *);
void test() { outer(inner); }

The patch adds a note:

t.cpp(4) : note: perhaps you meant to call this function with '()'?
void f2() { outer(inner); }
^
()

Any comments would be appreciated.

Thanks,
- Sam

func_as_arg.patch (2.79 KB)

This is a good start, and thanks for working on it! I have a few comments:

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

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.

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

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

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

  - Doug

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

+ // 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\-&gt;getAs&lt;FunctionType&gt;\(\)\-&gt;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

func_as_arg_v2.patch (5.66 KB)

Please don't do this. Consistency is important, and so is knowing what candidate the note is referring to. The code modification hint isn't worth losing that (not that you should remove the hint; it just won't get displayed by the console client).

John.

Ok, that makes sense. I've attached a new patch. Thanks for reviewing!

Thanks,
Sam

func_as_arg_v3.patch (5.22 KB)