[cfe-commits] PATCH #1 for "[sema++] clang should give a fixit for */& mismatch"

[Anna Zaks]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR5941_2.diff
Type: application/octet-stream
Size: 12565 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110718/04000d46/attachment.obj>
-------------- next part --------------


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