[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 Thu, Jan 24, 2013 at 2:19 PM, jahanian <fjahanian at apple.com> wrote:
>
> On Jan 23, 2013, at 8:28 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
>>>
>>> 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.
>>
>
>
> Typos fixed and patch checked in r173377. I also, compiled couple of the
> largest Sema files and timed them using our internal measurement tool and did not
> noticed any noticeable change. In the course of compiling these files,  I noticed a
> crash in the expression evaluator and fixed it using this additional change (which now
> successfully builds llvm).
>
> -void Sema::CheckCompletedExpr(Expr *E, SourceLocation CheckLoc) {
> +void Sema::CheckCompletedExpr(Expr *E, SourceLocation CheckLoc,
> +                              bool IsConstexpr) {
>   CheckImplicitConversions(E, CheckLoc);
>   CheckUnsequencedOperations(E);
> +  if (!IsConstexpr && !E->isValueDependent())
> +    CheckForIntOverflow(E);
> }
>
> Note the addition of check on expression's value dependency. I was leery to
> add this check deep inside the evaluator as I am not comfortable doing that.

Thanks, checking this here is fine.