[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
Richard Smith
richard at metafoo.co.uk
Tue Jul 19 16:23:13 PDT 2011
This improved diagnostic looks really promising. Some comments below.
On Tue, July 19, 2011 20:49, Anna Zaks 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/DiagnosticS
> emaKinds.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?r
> ev=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=1
> 35509&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;
This misses out on a lot of common cases where we should avoid suggesting
parentheses: x[n], f(x), it++, x.f, p->f. Ideally, we shouldn't suggest
parentheses for any postfix-expression.
> +
> + // Check if the argument needs to be dereferenced
> + // (type * -> type) or (type * -> type &).
> + if (const PointerType *FromPtrTy = dyn_cast<PointerType>(FromQTy)) {
It would be great if we could extend this logic to C++ class types with
overloaded operator*. This sort of mistake happens with iterators and smart
pointers fairly frequently in my experience. If you're not interested in
pursuing this, I'll add it to my queue.
> + // 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, "*"));
> + }
If the operand is &x (for a non-overloaded operator&), we should suggest
removing the &, not adding a *. (And vice versa below).
> + 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;
This asserts if too many FixIts are added, for instance if given:
void f(int*, int*);
void g(int *b) { f(*b, &b); }
Partly, this is because PartialDiagnostic::Storage::MaxFixItHints is still 3,
but you will need to avoid adding more fix-its than the diagnostics system can
cope with too. Possibly the best solution would be to make PartialDiagnostic
use a SmallVector.
> + S.Diag(Fn->getLocation(), FDiag);
It's a great shame that the fix-its will not usually appear when building with
clang from the command line. Can we split the fix-its to a separate note at
the location of the first fix-it?
> +
> 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;
gcc warns on this dangling else.
> +
> // 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 &
Shouldn't this be: CHECK-NOT: dereference 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
We should probably check that the base class is accessible: currently we give
a fix-it for this:
struct A {};
class B : A {};
void f(A *);
void g(B &b) { f(b); }
Best wishes,
Richard
More information about the cfe-commits
mailing list