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
Description: Binary data
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
