[cfe-commits] r172016 - in /cfe/trunk: include/clang/AST/Expr.h lib/AST/ExprConstant.cpp lib/Sema/SemaStmt.cpp test/Sema/switch-1.c

[Richard Smith]

On Wed, Jan 16, 2013 at 5:05 PM, jahanian <fjahanian at apple.com> wrote:
>
> On Jan 14, 2013, at 5:14 PM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>
>
> On Jan 14, 2013, at 4:55 PM, Fariborz Jahanian wrote:
>
>
> On Jan 14, 2013, at 1:30 PM, Richard Smith wrote:
>
> On Mon, Jan 14, 2013 at 12:23 PM, jahanian <fjahanian at apple.com> wrote:
>>
>>
>> On Jan 11, 2013, at 2:55 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> There are two different kinds of overflow that can happen in a case
>> expression:
>>
>> 1) An overflow occurs while evaluating the case value:
>>
>> switch (n)
>>   case 123456 * 123456:
>>
>> 2) An overflow occurs while converting the evaluated case value to the
>> promoted type of the switch expression:
>>
>> int n = /*...*/;
>> switch (n)
>>   case 1ULL << 45:
>>
>> For situation (1), it doesn't seem pertinent that this is happening within
>> a case expression; we should warn no matter where it happens; this is the
>> focus of PR2729. (2) should already be being checked by the call to
>> ConvertIntegerToTypeWarnOnOverflow.
>>
>>
>> This Patch checks and warns for overflow.
>
>
> Thanks!
>>
>> Yes, that would make sense to me, although we will need some mechanism to
>> suppress the warning in the case where the overflow produces an error (as
>> happens in constant expressions in C++11), and we'd need to avoid repeatedly
>> evaluating subexpressions.
>>
>>
>> I am afraid I don't see how  this warning can be avoided in case of
>> constant expressions in C++11. Expression evaluations are disjoint from
>> declarations specs.
>> For example, clang already issues this warning (and error for constant
>> expressions ) in the case of left-shift operations.
>> constexpr int ll = (2147483647 << 2);
>>
>> Can't we have the additional warning? Please advise.
>
>
> I think the additional warning is fine for the time being. In the longer
> term, we could perform this check in ActOnFinishFullExpr (alongside the call
> to CheckImplicitConversions), and suppress it if the full expression is
> required to be a constant expression.
>
> Does this patch have acceptable performance for large constant expressions?
> I'm a little concerned that it appears it will repeatedly evaluate
> subexpressions (the constant expression evaluator doesn't perform any
> caching).
>
>
> There is always some cost evaluating a constant expression looking for
> overflow. We already have this overhead for checking on
> '<<' operation (and I am using the same API for evaluating the constants for
> for +/-/*  checks)
> Good news is that if constant is represented by 'IntegerLiteral' node, then
> its 64bit representation is readily available. But, if we need to build a
> new IntegerLiteral, due to constant folding in the process, then this node
> need
> be built. But I have no way of knowing performance implication of evaluating
> the folded constants. I can't imagine that cost of such an overhead be too
> prohibitive
> to inhibit having overflow checking.
>
>
> I misspoke. Expression evaluator does not build new ASTs. It just evaluates.
> So, overhead of doing this is there, but I don't know how it can be
> avoided.
>
>
> Any more thought on this?

Here's one possible plan:

Add a mode to the expression evaluator which visits all subexpressions
and produces warnings for overflow etc. Call it in that mode from
CheckCompletedExpr (just added in r172690), except in cases where the
expression is required to be constant (the callers of
ActOnFinishFullExpr should know this).