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

[Daniel Dunbar]

On Mon, Nov 2, 2009 at 1:45 PM, Daniel Dunbar <daniel at zuster.org> wrote:
> 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.

I see you already added the tests, thanks!

 - Daniel

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