[cfe-commits] r85324 - in /cfe/trunk/lib: AST/ExprConstant.cpp CodeGen/CGBuiltin.cpp

[Mike Stump]

On Oct 29, 2009, at 10:49 AM, Daniel Dunbar wrote:
> I find this confusing, we now have two notions of "has side effects"
> in ExprConstant.cpp. Do these need to be distinct? At the least I'm
> surprised the visitor doesn't reuse the EvalResult one.

The existing one doesn't answer the question, does this expression  
have any side effects.  :-(  I was wishing it did.  For that reason, I  
can't use it.  I didn't see any common ground between them.

> Also, please include some test cases which exercise this stuff.

I have one in the testsuite, disguised as a _builtin_object_size  
test...  which was the main motivation for doing up the code.  I  
initially was just going to return true and put a FIXME to implement  
it, but I thought, oh well, let's just pound it all out now and not  
have a fixme.  I'll see about adding some more.

>> +  bool VisitArraySubscriptExpr(ArraySubscriptExpr *E)
>> +    { return Visit(E->getLHS()) && Visit(E->getRHS()); }
>
> This should be || ?

Yup, good catch.  Thanks.

>> +  bool VisitBinaryOperator(BinaryOperator *E) { return false; }
>
> This needs to visit the sub expressions?

Yes.

>> +  bool VisitUnaryDeref(UnaryOperator *E) {
>> +    if (E->getType().isVolatileQualified())
>> +      return true;
>> +    return false;
>> +  }
>
> So does this?

Yes, thanks.

These points fixed in r85526.