[cfe-commits] r55767 - in /cfe/trunk: include/clang/AST/ASTContext.h include/clang/Basic/DiagnosticKinds.def lib/AST/ASTContext.cpp lib/Sema/Sema.h lib/Sema/SemaExpr.cpp

[steve naroff]

Eli,

I'll fix these issues tomorrow (the type checker has changed  
considerably since this work was originally done).

Since you've made many of the recent type checker improvements, I'm  
grateful for your review.

Thanks,

snaroff

On Sep 4, 2008, at 3:14 PM, Eli Friedman wrote:

> On Thu, Sep 4, 2008 at 8:10 AM, Steve Naroff <snaroff at apple.com>  
> wrote:
>> +/// typesAreBlockCompatible - This routine is called when  
>> comparing two
>> +/// closure types. Types must be strictly compatible here.
>> +bool ASTContext::typesAreBlockCompatible(QualType lhs, QualType  
>> rhs) {
>> +  if (lhs.getCVRQualifiers() != rhs.getCVRQualifiers())
>> +    return false;
>> +
>> +  QualType lcanon = getCanonicalType(lhs);
>> +  QualType rcanon = getCanonicalType(rhs);
>
> You should be checking qualifiers on the canonical types; the
> comparson can get messed up for arrays otherwise.  Although, see
> below.
>
>> +  // If two types are identical, they are are compatible
>> +  if (lcanon == rcanon)
>> +    return true;
>> +  if (isa<FunctionType>(lcanon) && isa<FunctionType>(rcanon)) {
>> +    const FunctionType *lbase = cast<FunctionType>(lcanon);
>> +    const FunctionType *rbase = cast<FunctionType>(rcanon);
>> +
>> +    // First check the return types.
>> +    if (!typesAreBlockCompatible(lbase->getResultType(),rbase- 
>> >getResultType()))
>> +      return false;
>> +
>> +    // Return types matched, now check the argument types.
>> +    const FunctionTypeProto *lproto =  
>> dyn_cast<FunctionTypeProto>(lbase);
>> +    const FunctionTypeProto *rproto =  
>> dyn_cast<FunctionTypeProto>(rbase);
>> +
>> +    if (lproto && rproto) { // two C99 style function prototypes
>> +      unsigned lproto_nargs = lproto->getNumArgs();
>> +      unsigned rproto_nargs = rproto->getNumArgs();
>> +
>> +      if (lproto_nargs != rproto_nargs)
>> +        return false;
>> +
>> +      if (lproto->isVariadic() || rproto->isVariadic())
>> +        return false;
>> +
>> +      // The use of ellipsis agree...now check the argument types.
>> +      for (unsigned i = 0; i < lproto_nargs; i++)
>> +        if (!typesAreBlockCompatible(lproto->getArgType(i),
>> +                                     rproto->getArgType(i)))
>> +          return false;
>> +      return true;
>> +    }
>> +    return (!lproto && !rproto); // two K&R style function decls  
>> match.
>> +  }
>> +  return false;
>> +}
>
> It looks like you could delete all of the stuff after the "if (lcanon
> == rcanon) return true;" without affecting how the code works, unless
> I'm mistaken... for two types have identical return types and argument
> lists, they'd have to be identical anyways.
>
> You need to modify ASTContext::mergeTypes to handle block pointer
> types; otherwise, I think it'll explode with an assertion.  Adding a
> "case Type::BlockPointer: return false;" should be sufficient, unless
> I'm misreading typesAreBlockCompatible.
>
> And at that point, you can just replace all the calls to
> typesAreBlockCompatible with calls to typesAreCompatible.
>
> Potentially useful test (should print a warning about comparing
> incompatible pointers): int a() {void (^b)(void),void(^c)(int); return
> &b == &c;}
>
>> +    /// BlockVoidPointer - The assignment is between a closure  
>> pointer and
>> +    /// void*, we accept for now.
>
> I don't see any good reason to accept this; it'll just destroy the
> block pointer.
>
>> +  // For blocks we enforce that qualifiers are identical.
>> +  if (lhptee.getCVRQualifiers() != rhptee.getCVRQualifiers())
>> +    ConvTy = CompatiblePointerDiscardsQualifiers;
>
> I thought it was supposed to be illegal to qualify block types?
>
> (Although, actually, I recall making a comment about an earlier patch
> rejecting qualifiers on block pointer types... I don't think that ever
> got fixed.)
>
>> /// CheckAssignmentConstraints (C99 6.5.16) - This routine currently
>> /// has code to accommodate several GCC extensions when type checking
>> /// pointers. Here are some objectionable examples that GCC  
>> considers warnings:
>> @@ -1500,6 +1528,25 @@
>>
>>    if (isa<PointerType>(rhsType))
>>      return CheckPointerTypesForAssignment(lhsType, rhsType);
>> +
>> +    if (const BlockPointerType *BPT = rhsType- 
>> >getAsBlockPointerType())
>> +      if (BPT->getPointeeType()->isVoidType())
>> +        return BlockVoidPointer;
>> +
>> +    return Incompatible;
>> +  }
>> +
>> +  if (isa<BlockPointerType>(lhsType)) {
>> +    if (rhsType->isIntegerType())
>> +      return IntToPointer;
>
> IntToBlockPointer?
>
>> +    if (isa<BlockPointerType>(lhsType) &&
>> +        rhsType->getAsPointerType()->getPointeeType()->isVoidType())
>
> Isn't this an impossible condition, considering the location in the  
> function?
>
>>    return Incompatible;
>>  }
>>
>> @@ -1532,6 +1583,11 @@
>>    ImpCastExprToType(rExpr, lhsType);
>>    return Compatible;
>>  }
>> +
>> +  // We don't allow conversion of non-null-pointer constants to  
>> integers.
>> +  if (lhsType->isBlockPointerType() && rExpr->getType()- 
>> >isIntegerType())
>> +    return IntToBlockPointer;
>
> This shouldn't be necessary; you already handle this case in
> CheckAssignmentConstraints.
>
>>  // This check seems unnatural, however it is necessary to ensure  
>> the proper
>>  // conversion of functions/arrays. If the conversion were done for  
>> all
>>  // DeclExpr's (created by ActOnIdentifierExpr), it would mess up  
>> the unary
>> @@ -1849,6 +1905,21 @@
>>    ImpCastExprToType(rex, lType); // promote the pointer to pointer
>>    return Context.IntTy;
>>  }
>> +  // Handle block pointer types.
>> +  if (lType->isBlockPointerType() && rType->isBlockPointerType()) {
>> +    QualType lpointee = lType->getAsBlockPointerType()- 
>> >getPointeeType();
>> +    QualType rpointee = rType->getAsBlockPointerType()- 
>> >getPointeeType();
>> +
>> +    if (!LHSIsNull && !RHSIsNull &&
>> +        !Context.typesAreBlockCompatible(lpointee, rpointee)) {
>> +      Diag(loc, diag::err_typecheck_comparison_of_distinct_blocks,
>> +           lType.getAsString(), rType.getAsString(),
>> +           lex->getSourceRange(), rex->getSourceRange());
>> +    }
>> +    ImpCastExprToType(rex, lType); // promote the pointer to pointer
>> +    return Context.IntTy;
>> +  }
>
> Just to double-check, the comparison is roughly equivalent to memcmp
> for block pointers?
>
> The code as-is won't work correctly for cases like "int
> a(int(^b)(void)){return b  == (void*)0}".
>
> Some code I spotted while reviewing, not sure when it was committed:
>  // Handle block pointers.
>  if (lType->isBlockPointerType() && rType->isIntegerType()) {
>    if (!RHSIsNull)
>      Diag(loc, diag::ext_typecheck_comparison_of_pointer_integer,
>           lType.getAsString(), rType.getAsString(),
>           lex->getSourceRange(), rex->getSourceRange());
>    ImpCastExprToType(rex, lType); // promote the integer to pointer
>    return Context.IntTy;
>  }
>  if (lType->isIntegerType() && rType->isBlockPointerType()) {
>    if (!LHSIsNull)
>      Diag(loc, diag::ext_typecheck_comparison_of_pointer_integer,
>           lType.getAsString(), rType.getAsString(),
>           lex->getSourceRange(), rex->getSourceRange());
>    ImpCastExprToType(lex, rType); // promote the integer to pointer
>    return Context.IntTy;
>  }
>
> This should be merged with the checks above it.
>
> -Eli