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

[jahanian]

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?

- Thanks, Fariborz

> 
> - Fariborz
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130116/f5bbdcae/attachment.html>