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

[jahanian]

- Ping.

- Thanks, Fariborz

Begin forwarded message:

> From: jahanian <fjahanian at apple.com>
> Subject: Re: [cfe-commits] r172016 - in /cfe/trunk: include/clang/AST/Expr.h lib/AST/ExprConstant.cpp lib/Sema/SemaStmt.cpp test/Sema/switch-1.c
> Date: January 18, 2013 5:59:17 PM PST
> To: Richard Smith <richard at metafoo.co.uk>
> Cc: "cfe-commits at cs.uiuc.edu cfe" <cfe-commits at cs.uiuc.edu>
> 
> 
> 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.
> 
> 	
> 
> 
> - 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
> 
> _______________________________________________
> 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/20130122/c2535067/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch-intoverflow.txt
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130122/c2535067/attachment.txt>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130122/c2535067/attachment-0001.html>