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

[Daniel Dunbar]

On Thu, Oct 29, 2009 at 2:28 PM, Mike Stump <mrs at apple.com> wrote:
> 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.

It still seems to me like they should be merged to at least provide
the information via the same API. If clients of the other interface
need to the precise semantics it provides (which, in my cursory
glance, seem fairly useless) then it would be better to get back an
enum describing more information about the exact "side effects".

>> 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.

Ok. What I was looking for was something like our other tests for
constant evaluation that would at least cover all the cases that
handled by the side effect visitor.

 - Daniel

>>> +  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.
>