[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

[Eli Friedman]

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