[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 3:57 PM, jahanian <fjahanian at apple.com> wrote:

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

This patch should be feature complete.

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


- Thanks, Fariborz

> 
> Rest of your comments, are incorporated in this patch.
> 
> 	
> <patch-intoverflow.txt>
> 
> - 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.
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits