[cfe-commits] r45951 - in /cfe/trunk: AST/Expr.cpp test/Sema/conditional-expr.c

[Steve Naroff]

On Jan 13, 2008, at 10:04 PM, Chris Lattner wrote:

> On Jan 13, 2008, at 6:53 PM, Steve Naroff wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=45951&view=rev
>> Log:
>>
>> Rewrite Expr::isNullPointerConstant() to deal with multiple levels  
>> of explicit casts.
>>
>> Now, isNullPointerConstant() will return true for the following:  
>> "(void*)(double*)0"
>
> Unfortunately, this violates the C spec.  :(
>

I just reverted this patch.

If it violates the C spec and doesn't solve Bjorn & Seo's problems,  
then we need another solution.

snaroff

> How about only accepting this with -pedantic?
>
> Some specific things about the patch:
>
>> +/// Helper function for isNullPointerConstant. This routine skips  
>> all
>> +/// explicit casts, implicit casts and paren expressions.
>> +const Expr * getNullPointerConstantExpr(const Expr *exp) {
>
> Ok, you could make it iterative, but not a big deal.  Please rename  
> this to "LookThroughCasts" or something like that, as the function  
> has nothing to do with null (the name should reflect what it does,  
> not who the client is).  Should it only look through casts to  
> pointer type?  We don't want to accept something like (void*)(int) 
> (float)0 as a null pointer constant.
>
>> /// isNullPointerConstant - C99 6.3.2.3p3 -  Return true if this is  
>> either an
>> /// integer constant expression with the value zero, or if this is  
>> one that is
>> /// cast to void*.
>> bool Expr::isNullPointerConstant(ASTContext &Ctx) const {
>> +  const CastExpr *CE = dyn_cast<CastExpr>(this);
>> +  const Expr *e = getNullPointerConstantExpr(this);
>
> This implementation is overly complex.  I'd eliminate "CE" entirely,  
> and change the code to be:
>
> const Expr *e = getNullPointerConstantExpr(this);
> if (!e->isinteger) return false;
>
> return true see if it's an integer constant expr with value 0.
>
> Isn't that sufficient?
>
> -Chris