On Jul 18, 2011, at 3:12 PM, Anna Zaks wrote:
> Attached is the updated patch. It includes the code review comments + sorting
> of the diagnostics based on the fixit info.
>
> (No test for objective C since it will not result in the error, but a warning
> coming from a different location in the code. I'll add these fix to more
> errors/warnings later on.)
>
> Thanks to everyone for the suggestions!
> Anna.
> <PR5941_2.diff>
This looks great! Only two minor comments, then please go ahead and commit.
Index: include/clang/Sema/Overload.h
===================================================================
--- include/clang/Sema/Overload.h (revision 135431)
+++ include/clang/Sema/Overload.h (working copy)
@@ -528,6 +528,12 @@
ovl_fail_final_conversion_not_exact
};
+ enum OverloadFixItKind {
+ ovl_fixit_undefined = 0,
+ ovl_fixit_dereference,
+ ovl_fixit_take_address
+ };
+
These should be named according to LLVM conventions
(http://llvm.org/docs/CodingStandards.html#ll_naming), e.g., OFIK_Undefined,
OFIK_Dereference, OFIK_TakeAddress.
+ // Check if the pointer to the argument needs to be passed
+ // (type -> type *) or (type & -> type *).
+ if (const PointerType *ToPtrTy = dyn_cast<PointerType>(ToQTy)) {
+ // Only suggest taking address of L-values.
+ if (!Arg->isLValue())
+ return false;
+
+ OpaqueValueExpr TmpExpr(Arg->getExprLoc(), FromQTy, VK_LValue);
This expression should be an rvalue, since &foo is an rvalue.
- Doug
> On Jul 15, 2011, at 12:00 PM, Douglas Gregor wrote:
>
>>
>> On Jul 14, 2011, at 7:26 PM, Anna Zaks wrote:
>>
>>> This patch is a first step towards providing FixItHints in case a function
>>> call is missing one/several * or & operators (See http://llvm.org/PR5941).
>>>
>>> With this fix, the error message will display the function call. It will be
>>> followed by the list of notes - one for each possible candidate function
>>> declaration. If a function call can be fixed to match the particular
>>> declaration, a FixItHint will be provided along with the note.
>>
>> Very cool. Comments below.
>>
>>> I also bumped up the number of MaxFixItHints to 6 (we need 2 FixIts per
>>> incorrect argument).
>>>
>>> The remaining steps are:
>>> 1) Use FixIts when determining the order in which the notes should be
>>> displayed.
>>> 2) Find out if the same FixIts can be used with errors other then "error:
>>> no matching function for call to foo".
>>
>>
>> +def note_ovl_candidate_bad_conv_fixit : Note<"candidate "
>> + "%select{function|function|constructor|"
>> + "function |function |constructor |"
>> + "constructor (the implicit default constructor)|"
>> + "constructor (the implicit copy constructor)|"
>> + "constructor (the implicit move constructor)|"
>> + "function (the implicit copy assignment operator)|"
>> + "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; "
>> + "did you miss * or & operator%s6?">;
>>
>> The diagnostic should suggest either * or &, depending on what would
>> actually fix the code, rather than requiring the user to think about it
>> further. This could be done with some kind of select, e.g.,
>>
>> %select{|;dereference the argument with *|;take the address of the
>> argument with &}7
>>
>> which has the advantage of being extensible (e.g., in case we want to also
>> suggest things like () to turn a function expression into a function call)
>> and could even be tacked on to note_ovl_candidate_bad_conv rather than
>> creating a new diagnostic.
>>
>> Index: lib/Sema/SemaOverload.cpp
>> ===================================================================
>> --- lib/Sema/SemaOverload.cpp (revision 135234)
>> +++ lib/Sema/SemaOverload.cpp (working copy)
>> @@ -6730,6 +6730,54 @@
>>
>> namespace {
>>
>> +/// Update the Hints with FixIts to correct the given conversion. Return
>> true
>> +/// on success.
>> +static bool TryToFixBadConversion(const Sema &S,
>> + const ImplicitConversionSequence &Conv,
>> + llvm::SmallVector<FixItHint, 4> &Hints) {
>> + assert(Conv.isBad());
>> + 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());
>> +
>> + // Check if the argument needs to be dereferenced
>> + // (type * -> type) or (type * -> type &).
>> + if (const CanQual<PointerType> FromPtrTy = FromQTy->getAs<PointerType>())
>> {
>> + const CanQualType ToTy = ToQTy.getNonReferenceType();
>> + const CanQualType FromTy = FromPtrTy->getPointeeType();
>> + if (ToTy.getUnqualifiedType() == FromTy.getUnqualifiedType() &&
>> + ToTy.isAtLeastAsQualifiedAs(FromTy)) {
>>
>> It's probably worth loosening the type checking here slightly, to support
>> (for example) derived-to-base conversions and Objective-C object pointer
>> conversions. I suspect you'll want to factor that check out into a separate
>> routine, since a similar check is needed for &.
>>
>> + Hints.push_back(FixItHint::CreateInsertion(Begin, "*("));
>> + Hints.push_back(FixItHint::CreateInsertion(End, ")"));
>> + return true;
>> + }
>> + }
>>
>> The parentheses are necessary only in a few cases… can we perhaps whitelist
>> common kinds of expressions (DeclRefExprs, ParenExprs, etc.) where the
>> parentheses aren't needed, and then just emit the simple "*" in those cases?
>> We want to try fairly hard to introduce the simplest, correct Fix-It we can.
>>
>> +
>> + // Check if the pointer to the argument needs to be passed
>> + // (type -> type *) or (type & -> type *).
>> + if (const CanQual<PointerType> ToPtrTy = ToQTy->getAs<PointerType>()) {
>> + const CanQualType FromTy = FromQTy.getNonReferenceType();
>> + const CanQualType ToTy = ToPtrTy->getPointeeType();
>> + if (FromTy.getUnqualifiedType() == ToTy.getUnqualifiedType() &&
>> + ToTy.isAtLeastAsQualifiedAs(FromTy)) {
>> + Hints.push_back(FixItHint::CreateInsertion(Begin, "&("));
>> + Hints.push_back(FixItHint::CreateInsertion(End, ")"));
>> + return true;
>> + }
>> + }
>> +
>>
>> For this case, we'll also need to check whether the expression is an lvalue
>> (Expr::isLValue()), so that we don't produce a bogus fix-it for something
>> like:
>>
>> void f(int*);
>> void g() { f(17); } // can't apply the & operator here
>>
>> Your patch is looking quite good; thanks for working on this!
>>
>> - Doug
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
