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