<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Nick,<div><br></div><div>The error message, which was changed by the patch, is triggered when there is no good overload candidate that matches a call statement. The error format is to print the error message with a call instruction, followed by a list of notes - one per each overload candidate. Consider a more elaborate test case:</div><div><br></div><div><div><div>void Func(int *, int &) { }</div><div>void Func(int *, int *) {}</div><div>void test() {</div><div> int x;</div><div> Func(x, x);</div><div>}</div></div></div><div><br></div><div>It makes sense to associate a FixItHint with each candidate since different candidates require different FixIts. The problem is that the FixIt location points to a call instruction and candidate notes display function declarations. The only way to see the FixIt on command line is using -fdiagnostics-parseable-fixits. </div><div><br></div><div>That said, I think we could benefit from special casing the code not to go through the overload resolution in simple cases like the example you provided.</div><div><br></div><div>Thanks for the feedback,</div><div>Anna.<br><div><div>On Jul 19, 2011, at 1:28 PM, Nick Lewycky wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Thanks!<div><br></div><div>I tried testing this patch out since it fixes one of our bugs too, but found something odd. Here's our testcase:</div><div><br></div><div><div> void Func(int *) { }</div><div> void test() {</div>
<div> int x;</div><div> Func(x);</div><div> }</div><div><br></div><div>"clang -fsyntax-only" on that doesn't emit the fix-it, *but* if you run "clang -cc1 -fdiagnostics-parseable-fixits -x c++" then it does print:</div>
<div><br></div><div><div>fix-it:"b4070551.cc":{4:9-4:9}:"&"</div></div><div><br></div><div>I don't understand why it shows up with -fdiagnostics-parseable-fixits but not as a fixit with just -fsyntax-only? Those two really shouldn't be different. Could you please take a look?</div>
<div><br></div><div>Nick</div><br><div class="gmail_quote">On 19 July 2011 12:49, Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com">ganna@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Author: zaks<br>
Date: Tue Jul 19 14:49:12 2011<br>
New Revision: 135509<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=135509&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=135509&view=rev</a><br>
Log:<br>
Add FixItHints in case a C++ function call is missing * or & operators on one/several of it's parameters (addresses <a href="http://llvm.org/PR5941" target="_blank">http://llvm.org/PR5941</a>).<br>
<br>
Added:<br>
cfe/trunk/test/FixIt/fixit-function-call.cpp<br>
Modified:<br>
cfe/trunk/include/clang/Basic/Diagnostic.h<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/include/clang/Sema/Overload.h<br>
cfe/trunk/lib/Sema/SemaOverload.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/Diagnostic.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Diagnostic.h?rev=135509&r1=135508&r2=135509&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Diagnostic.h?rev=135509&r1=135508&r2=135509&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/Diagnostic.h (original)<br>
+++ cfe/trunk/include/clang/Basic/Diagnostic.h Tue Jul 19 14:49:12 2011<br>
@@ -615,7 +615,7 @@<br>
/// only support 10 ranges, could easily be extended if needed.<br>
CharSourceRange DiagRanges[10];<br>
<br>
- enum { MaxFixItHints = 3 };<br>
+ enum { MaxFixItHints = 6 };<br>
<br>
/// FixItHints - If valid, provides a hint with some code<br>
/// to insert, remove, or modify at a particular position.<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=135509&r1=135508&r2=135509&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=135509&r1=135508&r2=135509&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Jul 19 14:49:12 2011<br>
@@ -1592,7 +1592,9 @@<br>
"function (the implicit move assignment operator)|"<br>
"constructor (inherited)}0%1"<br>
" not viable: no known conversion from %2 to %3 for "<br>
- "%select{%ordinal5 argument|object argument}4">;<br>
+ "%select{%ordinal5 argument|object argument}4; "<br>
+ "%select{|dereference the argument with *|"<br>
+ "take the address of the argument with &}6">;<br>
def note_ovl_candidate_bad_addrspace : Note<"candidate "<br>
"%select{function|function|constructor|"<br>
"function |function |constructor |"<br>
<br>
Modified: cfe/trunk/include/clang/Sema/Overload.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Overload.h?rev=135509&r1=135508&r2=135509&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Overload.h?rev=135509&r1=135508&r2=135509&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Sema/Overload.h (original)<br>
+++ cfe/trunk/include/clang/Sema/Overload.h Tue Jul 19 14:49:12 2011<br>
@@ -528,6 +528,12 @@<br>
ovl_fail_final_conversion_not_exact<br>
};<br>
<br>
+ enum OverloadFixItKind {<br>
+ OFIK_Undefined = 0,<br>
+ OFIK_Dereference,<br>
+ OFIK_TakeAddress<br>
+ };<br>
+<br>
/// OverloadCandidate - A single candidate in an overload set (C++ 13.3).<br>
struct OverloadCandidate {<br>
/// Function - The actual function that this candidate<br>
@@ -556,6 +562,21 @@<br>
/// function arguments to the function parameters.<br>
llvm::SmallVector<ImplicitConversionSequence, 4> Conversions;<br>
<br>
+ /// The FixIt hints which can be used to fix the Bad candidate.<br>
+ struct FixInfo {<br>
+ /// The list of Hints (all have to be applied).<br>
+ llvm::SmallVector<FixItHint, 4> Hints;<br>
+<br>
+ /// The number of Conversions fixed. This can be different from the size<br>
+ /// of the Hints vector since we allow multiple FixIts per conversion.<br>
+ unsigned NumConversionsFixed;<br>
+<br>
+ /// The type of fix applied.<br>
+ OverloadFixItKind Kind;<br>
+<br>
+ FixInfo(): NumConversionsFixed(0), Kind(OFIK_Undefined) {}<br>
+ } Fix;<br>
+<br>
/// Viable - True to indicate that this overload candidate is viable.<br>
bool Viable;<br>
<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaOverload.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=135509&r1=135508&r2=135509&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=135509&r1=135508&r2=135509&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaOverload.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaOverload.cpp Tue Jul 19 14:49:12 2011<br>
@@ -6732,6 +6732,85 @@<br>
<br>
namespace {<br>
<br>
+/// Try to find a fix for the bad conversion. Populate the ConvFix structure<br>
+/// on success. Produces the hints for the following cases:<br>
+/// - The user forgot to apply * or & operator to one or more arguments.<br>
+static bool TryToFixBadConversion(Sema &S,<br>
+ const ImplicitConversionSequence &Conv,<br>
+ OverloadCandidate::FixInfo &ConvFix) {<br>
+ assert(Conv.isBad() && "Only try to fix a bad conversion.");<br>
+<br>
+ const Expr *Arg = Conv.Bad.FromExpr;<br>
+ if (!Arg)<br>
+ return false;<br>
+<br>
+ // The conversion is from argument type to parameter type.<br>
+ const CanQualType FromQTy = S.Context.getCanonicalType(Conv.Bad<br>
+ .getFromType());<br>
+ const CanQualType ToQTy = S.Context.getCanonicalType(Conv.Bad.getToType());<br>
+ const SourceLocation Begin = Arg->getSourceRange().getBegin();<br>
+ const SourceLocation End = S.PP.getLocForEndOfToken(Arg->getSourceRange()<br>
+ .getEnd());<br>
+ bool NeedParen = true;<br>
+ if (isa<ParenExpr>(Arg) || isa<DeclRefExpr>(Arg))<br>
+ NeedParen = false;<br>
+<br>
+ // Check if the argument needs to be dereferenced<br>
+ // (type * -> type) or (type * -> type &).<br>
+ if (const PointerType *FromPtrTy = dyn_cast<PointerType>(FromQTy)) {<br>
+ // Try to construct an implicit conversion from argument type to the<br>
+ // parameter type.<br>
+ OpaqueValueExpr TmpExpr(Arg->getExprLoc(), FromPtrTy->getPointeeType(),<br>
+ VK_LValue);<br>
+ ImplicitConversionSequence ICS =<br>
+ TryCopyInitialization(S, &TmpExpr, ToQTy, true, true, false);<br>
+<br>
+ if (!ICS.isBad()) {<br>
+ // Do not suggest dereferencing a Null pointer.<br>
+ if (Arg->IgnoreParenCasts()-><br>
+ isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull))<br>
+ return false;<br>
+<br>
+ if (NeedParen) {<br>
+ ConvFix.Hints.push_back(FixItHint::CreateInsertion(Begin, "*("));<br>
+ ConvFix.Hints.push_back(FixItHint::CreateInsertion(End, ")"));<br>
+ } else {<br>
+ ConvFix.Hints.push_back(FixItHint::CreateInsertion(Begin, "*"));<br>
+ }<br>
+ ConvFix.NumConversionsFixed++;<br>
+ if (ConvFix.NumConversionsFixed == 1)<br>
+ ConvFix.Kind = OFIK_Dereference;<br>
+ return true;<br>
+ }<br>
+ }<br>
+<br>
+ // Check if the pointer to the argument needs to be passed<br>
+ // (type -> type *) or (type & -> type *).<br>
+ if (isa<PointerType>(ToQTy)) {<br>
+ // Only suggest taking address of L-values.<br>
+ if (!Arg->isLValue())<br>
+ return false;<br>
+<br>
+ OpaqueValueExpr TmpExpr(Arg->getExprLoc(),<br>
+ S.Context.getPointerType(FromQTy), VK_RValue);<br>
+ ImplicitConversionSequence ICS =<br>
+ TryCopyInitialization(S, &TmpExpr, ToQTy, true, true, false);<br>
+ if (!ICS.isBad()) {<br>
+ if (NeedParen) {<br>
+ ConvFix.Hints.push_back(FixItHint::CreateInsertion(Begin, "&("));<br>
+ ConvFix.Hints.push_back(FixItHint::CreateInsertion(End, ")"));<br>
+ } else {<br>
+ ConvFix.Hints.push_back(FixItHint::CreateInsertion(Begin, "&"));<br>
+ }<br>
+ ConvFix.NumConversionsFixed++;<br>
+ if (ConvFix.NumConversionsFixed == 1)<br>
+ ConvFix.Kind = OFIK_TakeAddress;<br>
+ return true;<br>
+ }<br>
+ }<br>
+ return false;<br>
+}<br>
+<br>
void DiagnoseBadConversion(Sema &S, OverloadCandidate *Cand, unsigned I) {<br>
const ImplicitConversionSequence &Conv = Cand->Conversions[I];<br>
assert(Conv.isBad());<br>
@@ -6903,10 +6982,21 @@<br>
}<br>
<br>
// TODO: specialize more based on the kind of mismatch<br>
- S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_conv)<br>
- << (unsigned) FnKind << FnDesc<br>
+<br>
+ // Emit the generic diagnostic and, optionally, add the hints to it.<br>
+ PartialDiagnostic FDiag = S.PDiag(diag::note_ovl_candidate_bad_conv);<br>
+ FDiag << (unsigned) FnKind << FnDesc<br>
<< (FromExpr ? FromExpr->getSourceRange() : SourceRange())<br>
- << FromTy << ToTy << (unsigned) isObjectArgument << I+1;<br>
+ << FromTy << ToTy << (unsigned) isObjectArgument << I + 1<br>
+ << (unsigned) (Cand->Fix.Kind);<br>
+<br>
+ // If we can fix the conversion, suggest the FixIts.<br>
+ for (llvm::SmallVector<FixItHint, 4>::iterator<br>
+ HI = Cand->Fix.Hints.begin(), HE = Cand->Fix.Hints.end();<br>
+ HI != HE; ++HI)<br>
+ FDiag << *HI;<br>
+ S.Diag(Fn->getLocation(), FDiag);<br>
+<br>
MaybeEmitInheritedConstructorNote(S, Fn);<br>
}<br>
<br>
@@ -7256,6 +7346,18 @@<br>
if (R->FailureKind != ovl_fail_bad_conversion)<br>
return true;<br>
<br>
+ // The conversion that can be fixed with a smaller number of changes,<br>
+ // comes first.<br>
+ unsigned numLFixes = L->Fix.NumConversionsFixed;<br>
+ unsigned numRFixes = R->Fix.NumConversionsFixed;<br>
+ numLFixes = (numLFixes == 0) ? UINT_MAX : numLFixes;<br>
+ numRFixes = (numRFixes == 0) ? UINT_MAX : numRFixes;<br>
+ if (numLFixes != numRFixes)<br>
+ if (numLFixes < numRFixes)<br>
+ return true;<br>
+ else<br>
+ return false;<br>
+<br>
// If there's any ordering between the defined conversions...<br>
// FIXME: this might not be transitive.<br>
assert(L->Conversions.size() == R->Conversions.size());<br>
@@ -7300,7 +7402,7 @@<br>
};<br>
<br>
/// CompleteNonViableCandidate - Normally, overload resolution only<br>
-/// computes up to the first<br>
+/// computes up to the first. Produces the FixIt set if possible.<br>
void CompleteNonViableCandidate(Sema &S, OverloadCandidate *Cand,<br>
Expr **Args, unsigned NumArgs) {<br>
assert(!Cand->Viable);<br>
@@ -7308,14 +7410,21 @@<br>
// Don't do anything on failures other than bad conversion.<br>
if (Cand->FailureKind != ovl_fail_bad_conversion) return;<br>
<br>
+ // We only want the FixIts if all the arguments can be corrected.<br>
+ bool Unfixable = false;<br>
+<br>
// Skip forward to the first bad conversion.<br>
unsigned ConvIdx = (Cand->IgnoreObjectArgument ? 1 : 0);<br>
unsigned ConvCount = Cand->Conversions.size();<br>
while (true) {<br>
assert(ConvIdx != ConvCount && "no bad conversion in candidate");<br>
ConvIdx++;<br>
- if (Cand->Conversions[ConvIdx - 1].isBad())<br>
+ if (Cand->Conversions[ConvIdx - 1].isBad()) {<br>
+ if ((Unfixable = !TryToFixBadConversion(S, Cand->Conversions[ConvIdx - 1],<br>
+ Cand->Fix)))<br>
+ Cand->Fix.Hints.clear();<br>
break;<br>
+ }<br>
}<br>
<br>
if (ConvIdx == ConvCount)<br>
@@ -7360,16 +7469,26 @@<br>
// Fill in the rest of the conversions.<br>
unsigned NumArgsInProto = Proto->getNumArgs();<br>
for (; ConvIdx != ConvCount; ++ConvIdx, ++ArgIdx) {<br>
- if (ArgIdx < NumArgsInProto)<br>
+ if (ArgIdx < NumArgsInProto) {<br>
Cand->Conversions[ConvIdx]<br>
= TryCopyInitialization(S, Args[ArgIdx], Proto->getArgType(ArgIdx),<br>
SuppressUserConversions,<br>
/*InOverloadResolution=*/true,<br>
/*AllowObjCWritebackConversion=*/<br>
S.getLangOptions().ObjCAutoRefCount);<br>
+ // Store the FixIt in the candidate if it exists.<br>
+ if (!Unfixable && Cand->Conversions[ConvIdx].isBad())<br>
+ Unfixable = !TryToFixBadConversion(S, Cand->Conversions[ConvIdx],<br>
+ Cand->Fix);<br>
+ }<br>
else<br>
Cand->Conversions[ConvIdx].setEllipsis();<br>
}<br>
+<br>
+ if (Unfixable) {<br>
+ Cand->Fix.Hints.clear();<br>
+ Cand->Fix.NumConversionsFixed = 0;<br>
+ }<br>
}<br>
<br>
} // end anonymous namespace<br>
<br>
Added: cfe/trunk/test/FixIt/fixit-function-call.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/FixIt/fixit-function-call.cpp?rev=135509&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/FixIt/fixit-function-call.cpp?rev=135509&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/FixIt/fixit-function-call.cpp (added)<br>
+++ cfe/trunk/test/FixIt/fixit-function-call.cpp Tue Jul 19 14:49:12 2011<br>
@@ -0,0 +1,87 @@<br>
+// RUN: %clang_cc1 -fdiagnostics-parseable-fixits -x c++ %s 2> %t || true<br>
+// RUN: FileCheck %s < %t<br>
+// PR5941<br>
+// END.<br>
+<br>
+/* Test fixits for * and & mismatch in function arguments.<br>
+ * Since fixits are on the notes, they cannot be applied automatically. */<br>
+<br>
+typedef int intTy;<br>
+typedef int intTy2;<br>
+<br>
+void f0(int *a);<br>
+void f1(double *a);<br>
+void f1(intTy &a);<br>
+<br>
+void f2(intTy2 *a) {<br>
+// CHECK: error: no matching function for call to 'f1<br>
+// CHECK: dereference the argument with *<br>
+// CHECK: void f1(intTy &a);<br>
+// CHECK: fix-it{{.*}}*(<br>
+// CHECK-NEXT: fix-it{{.*}})<br>
+// CHECK: void f1(double *a);<br>
+ f1(a + 1);<br>
+<br>
+// This call cannot be fixed since without resulting in null pointer dereference.<br>
+// CHECK: error: no matching function for call to 'f1<br>
+// CHECK-NOT: take the address of the argument with &<br>
+// CHECK-NOT: fix-it<br>
+ f1((int *)0);<br>
+}<br>
+<br>
+void f3(int &a) {<br>
+// CHECK: error: no matching function for call to 'f0<br>
+// CHECK: fix-it{{.*}}&<br>
+ f0(a);<br>
+}<br>
+<br>
+<br>
+void m(int *a, const int *b); // match 2<br>
+void m(double *a, int *b); // no match<br>
+void m(int *a, double *b); // no match<br>
+void m(intTy &a, int *b); // match 1<br>
+<br>
+void mcaller(intTy2 a, int b) {<br>
+// CHECK: error: no matching function for call to 'm<br>
+// CHECK: take the address of the argument with &<br>
+// CHECK: fix-it{{.*}}&<br>
+// CHECK: take the address of the argument with &<br>
+// CHECK: fix-it{{.*}}&<br>
+// CHECK: fix-it{{.*}}&<br>
+ m(a, b);<br>
+<br>
+// This call cannot be fixed because (a + 1) is not an l-value.<br>
+// CHECK: error: no matching function for call to 'm<br>
+// CHECK-NOT: fix-it<br>
+ m(a + 1, b);<br>
+}<br>
+<br>
+// Test derived to base conversions.<br>
+struct A {<br>
+ int xx;<br>
+};<br>
+<br>
+struct B : public A {<br>
+ double y;<br>
+};<br>
+<br>
+bool br(A &a);<br>
+bool bp(A *a);<br>
+bool dv(B b);<br>
+<br>
+void dbcaller(A *ptra, B *ptrb) {<br>
+ B b;<br>
+<br>
+// CHECK: error: no matching function for call to 'br<br>
+// CHECK: fix-it{{.*}}*<br>
+ br(ptrb); // good<br>
+// CHECK: error: no matching function for call to 'bp<br>
+// CHECK: fix-it{{.*}}&<br>
+ bp(b); // good<br>
+<br>
+// CHECK: error: no matching function for call to 'dv<br>
+// CHECK-NOT: fix-it<br>
+ dv(ptra); // bad: base to derived<br>
+}<br>
+<br>
+// CHECK: errors generated<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</blockquote></div><br></div></body></html>