[cfe-commits] r59196 - in /cfe/trunk: lib/AST/Expr.cpp lib/AST/ExprConstant.cpp test/Sema/const-eval.c

[Daniel Dunbar]

Hmm, perhaps I did not do a good job explaining. However, I don't
understand why you reverted... can you explain what you think is wrong
with this approach? I have no problem doing it differently, I was just
trying to address regressions I was seeing...

The problem I was trying to solve was this:

>>  - Evaluation of , operator used bogus assumption that LHS could be
>>   evaluated as an integral expression even though its type is
>>   unspecified.

This causes assertions in other parts of the code because it tries to
do silly things like get the bit width of void.

On Wed, Nov 12, 2008 at 5:19 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> Woah, this looks really strange.

Not sure why, the code is basically the same it just avoids the
traversal of the LHS in isICE. As I mentioned, this is overly
permissive, but it isn't clear how to resolve this in isICE.

> One, you're committing a change to isICE without a test, which is
> definitely a bad idea.  What are you trying to accomplish there?

There was a test case added to const-eval.c.

> Second, this is somewhat tricky territory with tryEvaluate: we can
> definitively calculate the value of a comma operator without it
> actually being constant.  I'm not sure how we want to deal with this
> case, though: I think a lot of the code using tryEvaluate is ignoring
> the possibility of side-effects.

The change wasn't about addressing side effects... the code for
tryEvaluate was very similar in effect (at least, that was my intent)
to what was there before. I will investigate why it was failing on the
test you added, however, note that now clang is asserting again on the
test I added. :)

I'm happy if you want to fix in a different fashion...

 - Daniel

>
> -Eli
>