[cfe-commits] r71145 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/block-misc.c test/SemaObjC/conditional-expr-2.m

[Mike Stump]

On May 6, 2009, at 9:11 PM, Eli Friedman wrote:
> On Wed, May 6, 2009 at 8:14 PM, Mike Stump <mrs at apple.com> wrote:
> This makes the handling for block pointers and function pointers
> inconsistent; is that the intent?

Yes.  block pointers are more akin to object pointers.  The model to  
use is a pointer to an object that itself contains a function pointer.

> (It's fine if it isn't, but I'd like confirmation.)  In any case, I  
> don't think you want to be
> creating a block pointer with a qualified function type.

The compiler claims that isn't what happens.  Thoughts?  When I run:

   void (^bp)(int);
   const void *vp = bp;

   f(1 ? vp : bp);

into the compiler, it says the destType is void *.

I know what people _want_ it to do, but it doesn't appear that that  
has been done yet.  When it is, yes, quite possibly is it better to  
not have any qualifiers on it.

>> @@ -3671,7 +3680,7 @@
>>   QualType lType = lex->getType();
>>   QualType rType = rex->getType();
>>
>> -  if (!lType->isFloatingType()) {
>> +  if (!lType->isFloatingType() && !(lType->isBlockPointerType() &&  
>> isRelational)) {
>
> This looks strange... if the issue is that we don't want to warn in
> cases of errors, the code should be reorganized in a more general way.

I think when if conditionals have 1, 2 or 3 parts, I think they are  
fairly simple, though, I can read and keep track of conditionals that  
span 20+ parts, thanks to gcc.  I do agree, there is an upper bound we  
should not cross, and I'd claim, 20 is far to large.  Where do people  
think the limit should be?

Would a bool just above be better, or a predicate function that given  
the type and isRelational decides wether we should possibly warn?  I  
kinda like the function, if we split it out.  Or, is it the ! and the  
&&s that you find confusing.  Possibly you'd find !(a || (b && c)) to  
be more understandable?

>> @@ -3867,14 +3881,20 @@
>>   }
>>   // Handle block pointers.
>>   if (lType->isBlockPointerType() && rType->isIntegerType()) {
>> -    if (!RHSIsNull)
>> +    if (isRelational)
>> +      Diag(Loc, diag::err_typecheck_invalid_operands)
>> +        << lType << rType << lex->getSourceRange() << rex- 
>> >getSourceRange();
>> +    else if (!RHSIsNull)
>
> Couldn't you just check isRelational in the outer if statement, and
> fall through for the error case?

Ah, yes, that's much better, thanks.  Oh, actually, on second thought,  
this code is way to lax.  I've tightened it up.  Check out r71171.