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

+  void EvaluateForOverflow(const ASTContext &Ctx,
+                            SmallVectorImpl<PartialDiagnosticAt> *Diag) const;

Extra space on second line.

+      // Sould return true in IntOverflowCheckMode, so that we check
for overflow
+      // even if some subexpressions can't be evaluated as constants.
+      // subexpressions can't be evaluated as constants.

Typo 'Sould' and extra line.

Other than that, this looks fine, assuming there's no performance hit.

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