[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 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?

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.