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

[Douglas Gregor]

On Mar 11, 2010, at 3:13 PM, Samuel Harrington wrote:

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


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