[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
Wed Jul 20 17:52:24 PDT 2011
Richard,
Thanks for the thorough review!
See the responces below.
Anna.
On Jul 19, 2011, at 4:23 PM, Richard Smith wrote:
> 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.
Added more checks in r135643.
>
>> +
>> + // 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.
I am not planning on spending much more time on this, but would be great if you are interested in working on this.
>
>> + // 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).
In r135643.
>
>> + 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.
>
Bumped up the number for now, but SmallVector sounds like the way to go (may do it later on).
>> + 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?
There are positives and negatives of providing the separate note on the command line.
If we add an extra note per fixit, the error messages could get very verbose. In addition, we do modify each note to specify how it should be fixed, so some information about how the problem should be fixed is there.
>
>> +
>> 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.
In r135643.
>
>> +
>> // 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 * ?
>
In r135643.
>> +// 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); }
>
Added TODO in r135643.
> Best wishes,
> Richard
>
More information about the cfe-commits
mailing list