[cfe-commits] [PATCH] for Bug7505 and three other related issues

[Douglas Gregor]

Hi Faisal,

Were you planning to finish up this patch? It's definitely going in the right direction.

	- Doug

On Sep 20, 2010, at 11:41 AM, Douglas Gregor wrote:

> 
> On Sep 10, 2010, at 3:26 PM, Faisal Vali wrote:
> 
>> The patch fixes the following related bugs:
>> 
>> 
>> 1) Fix bug http://llvm.org/bugs/show_bug.cgi?id=7505
>>  (overload resolution of &foo<int> when it identifies a single function)
>> 
>> template<class T> void foo(T);
>> bool b = &foo<int>; // now, ok.
>> 
>> -- added a call to Sema::ResolveSingleFunctionTemplateSpecialization in
>>  Sema::ResolveAddressOfOverloadedFunction prior to any other deduction
>>  or resolution to check to see if the name identifies a single function
>>  (SemaOverload.cpp)
> 
> This is:
> 
> @@ -6225,19 +6244,49 @@
>   OverloadExpr::FindResult Ovl = OverloadExpr::find(From);
>   OverloadExpr *OvlExpr = Ovl.Expression;
> 
> +  // template<class T> foo(T); &foo<int>; can only identify one function
> +  // C++0x [temp.arg.explicit]p3:
> +    //   [...] In contexts where deduction is done and fails, or in contexts
> +    //   where deduction is not done, if a template argument list is 
> +    //   specified and it, along with any default template arguments, 
> +    //   identifies a single function template specialization, then the 
> +    //   template-id is an lvalue for the function template specialization.
> +  if (FunctionType->isBooleanType() && OvlExpr->hasExplicitTemplateArgs())
> +  {
> +    if (FunctionDecl* Fun = ResolveSingleFunctionTemplateSpecialization(From, 
> +                                    Complain, &FoundResult) )
> +    {
> +      MarkDeclarationReferenced(From->getLocStart(), Fun);
> +      if (Complain) {
> +        CheckUnresolvedAccess(*this, OvlExpr, FoundResult);
> +        DiagnoseUseOfDecl(FoundResult, OvlExpr->getNameLoc());
> +      }
> +      return Fun;
> +    }
> +    return 0;
> +  }
> +
> 
> but it looks like it's in the wrong place. [temp.arg.explicit]p3 only kicks in where deduction is not done or where it fails, so it seems that this code should come at the end of this routine, after we've tried (and failed) to do deduction.
> 
> Also, I don't understand why we have the FunctionType->isBooleanType() check, which is rather... unintuitive. Why check for 'bool' and not 'int', for example? 
> 
>> -- modified Sema.h to add a 'Complain' flag to getMostSpecialized and
>>  ResolveSingleFunctionTemplateSpecialization so that we can percolate
>>  it through to these other functions that do Complain even if we
>>  don't intend to from ResolveAddressOfOverloadedFunction
>>  (required adding another call to ResolveOverload in SemaExpr.cpp with
>>   Complain set to true, since PerformImplicitConversion was calling
>>   it with 'false' yet the function was still complaining)
>>   (Sema.h, SemaTemplateDeduction.cpp)
> 
> Okay.
> 
>> -- made some cosmetic changes (left the major refactoring for the next revision)
>>  to ResolveSingleFunctionTemplateSpecialization
>> 
>> 
>> 
>> 2) Fix invalid conversion of an unresolved overloaded name into bool via '!'
>> template<class T> void foo(T);
>> bool b = !&foo;  // this should be ambiguous
>> -- Added a call to PerformContextuallyConvertToBool in CreateBuiltinUnaryOp
>>    to ensure that the OverloadTy does not leak through (semaExpr.cpp)
> 
> That's is code:
> 
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> --- lib/Sema/SemaExpr.cpp	(revision 113599)
> +++ lib/Sema/SemaExpr.cpp	(working copy)
> @@ -6771,9 +6781,26 @@
> 
>     resultType = Input->getType();
>     if (resultType->isDependentType())
>       break;
> -    if (!resultType->isScalarType()) // C99 6.5.3.3p1
> +    
> +    if (!resultType->isScalarType())
> +    {
>       return ExprError(Diag(OpLoc, diag::err_typecheck_unary_expr)
>         << resultType << Input->getSourceRange());
> +    }
> +    // ensure that it is convertible to bool
> +    // i.e weed out '!&f' when &f is overloaded, but allow &f<int>
> +    if (PerformContextuallyConvertToBool(Input)) // C99 6.5.3.3p1
> +    {
> +      if (resultType == Context.OverloadTy)
> +      {
> +        DeclAccessPair access;
> +        // Call it again (initially was thru PerformContext) but now Complain
> +        ResolveAddressOfOverloadedFunction(Input, Context.BoolTy, 
> +              /* Complain */ true, access);
> +        
> +      }
> +      return ExprError(); // Diagnostic is uttered above
> +    }
> 
> Why not push the diagnostics for overloaded functions into PerformContextuallyConvertToBool? Then all clients of PerformContextuallyConvertToBool would benefit.
> 
>> 3) Fix for a better error message when we can't resolve a function name
>> - When TryImplicitConversion fails in InitializationSequence Constructor
>>   before calling it a conversion failure, check to see if it could resolve
>>   the name, and if it can't then it is an overloadresolution failure
>>  (SemaInit.cpp)
> 
> This looks good.
> 
>> 4) Fix a bug that allowed invalid deduction during a c-style cast to a
>> function reference
>> - template<class T> void f(T);
>> - (void)((void (&)(int,char))f); // this would compile and should error!!
>> - (void)((void (*)(int,char))f);  // error as expected
>> - In 'TryStaticImplicitCast' before allowing a cstyle cast if a
>> simple static cast failed,
>>   make sure we are not trying to cstyle cast a 'name' that could not
>> be resolved
>>   using the information in the cast (i.e the destination signature)
>>   (SemaCXXCast.cpp)
> 
> I think this is the code here:
> 
> Index: lib/Sema/SemaCXXCast.cpp
> ===================================================================
> --- lib/Sema/SemaCXXCast.cpp	(revision 113599)
> +++ lib/Sema/SemaCXXCast.cpp	(working copy)
> @@ -962,7 +962,17 @@
>   InitializationSequence InitSeq(Self, Entity, InitKind, &SrcExpr, 1);
>   if (InitSeq.getKind() == InitializationSequence::FailedSequence && 
>       (CStyle || !DestType->isReferenceType()))
> +  {
> +    // Even a cstyle cast can not convert (or reinterpret) a 
> +    // name that can not be resolved
> +    // do not allow - template<class T> void f(T);
> 
> +    // (void)((void (&)(int,char))f);
> +    if (!(SrcExpr->getType() == Self.Context.OverloadTy && 
> +      InitSeq.getFailureKind() == 
> +        InitializationSequence::FK_AddressOfOverloadFailed))
> +    
>     return TC_NotApplicable;
> +  }
> 
> but this isn't the right approach to a fix. When the implicit cast fails, and we have other options (because this is a C-style cast or isn't binding to a reference), this function is right to return TC_NotApplicable and the caller will try different casts. What's happening with the example code is that we're (correctly) returning TC_NotApplicable, but then TryReinterpretCast is allowing the bogus cast. We need to fix TryReinterpretCast not to allow it.
> 
> Other comments..
> 
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> --- lib/Sema/SemaExpr.cpp	(revision 113599)
> +++ lib/Sema/SemaExpr.cpp	(working copy)
> @@ -4962,7 +4962,17 @@
> 
>       // cv-unqualified type of the left operand.
>       if (PerformImplicitConversion(rExpr, lhsType.getUnqualifiedType(),
>                                     AA_Assigning))
> +      {
> +        if (rExpr->getType() == Context.OverloadTy)
> +        {
> +          
> +          DeclAccessPair tmp;
> +          // Call it again (initially was thru PerformImplict) but now Complain
> +          ResolveAddressOfOverloadedFunction(rExpr, lhsType
> +                                    , /* Complain */ true, tmp);
> +        }
>         return Incompatible;
> +      }
>       return Compatible;
>     }
> 
> The extra diagnostics for a failed implicit conversion from an overloaded function name should be moved into PerformImplicitConversion, rather than requiring all of the callers to be updated.
> 
> 	- Doug