[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 18, 2013, at 11:59 AM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Fri, Jan 18, 2013 at 11:06 AM, jahanian <fjahanian at apple.com> wrote:
>> 
>> On Jan 17, 2013, at 3:24 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> 
>>> This looks like a good start. But I think it won't check for integer
>>> overflow in subexpressions of a full-expression? eg, "case (123456 *
>>> 789012) + 1:" In order to catch that without repeatedly evaluating
>>> subexpressions, I was thinking you could move the checking logic into
>>> ExprConstant.cpp.
>>> 
>> 
>> You were correct, it wasn't. So, I moved the checking logic into ExprConstant.cpp, using the evaluator
>> already available there. Here is my latest patch.
> 
> Thanks, this looks good. A few things:
> 
> It'd be nice for the diagnostic to be a bit more informative; maybe
> include the source value and destination type like HandleOverflow
> does?

I haven't done the above yet. But, it shouldn't be controversial and will go into the final patch.

Rest of your comments, are incorporated in this patch.

	
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch-intoverflow.txt
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130118/1413d54d/attachment.txt>
-------------- next part --------------


- Thanks, Fariborz

> 
> EvalInfo::keepEvaluatingAfterFailure should return true in
> IntOverflowCheckMode, so that we check for overflow even if some
> subexpressions can't be evaluated as constants. Likewise, either you
> should suppress other diagnostics in this mode, or (preferably) just
> directly emit the warning from ExprConstant.cpp; currently, if some
> other part of the expression evaluator emits a "could not evaluate"
> note first, it'll suppress your warning, and we will only emit one
> warning even if there are multiple instances of overflow within the
> expression.
> 
> The name 'EvaluateKnownConstIntForOverflow' isn't right -- we're not
> dealing with an expression which is known to be an ICE. Just
> 'EvaluateForOverflow'?
> 
> In CPlusPlus11, IsConstexpr should be 'true' for the calls to
> ActOnFinishFullExpr in ActOnCaseStmt.
> 
> I would prefer you factored out the shared code between
> Expr::EvaluateAsRValue and Expr::EvaluateForOverflow rather than
> adding a new parameter to Expr::EvaluateAsRValue.