[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 10, 2013 at 12:31 PM, jahanian <fjahanian at apple.com> wrote:
>
> On Jan 9, 2013, at 5:58 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
>> On Wed, Jan 9, 2013 at 3:04 PM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>>> Author: fjahanian
>>> Date: Wed Jan  9 17:04:56 2013
>>> New Revision: 172016
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=172016&view=rev
>>> Log:
>>> Issue warning when case value is too large to fit
>>> in case condition type. // rdar://11577384.
>>> Test is conditionalized on x86_64-apple triple as
>>> I am not sure if the INT_MAX/LONG_MAX values in the test
>>> will pass this test for other hosts.
>>>
>>> Added:
>>>    cfe/trunk/test/Sema/switch-1.c
>>> Modified:
>>>    cfe/trunk/include/clang/AST/Expr.h
>>>    cfe/trunk/lib/AST/ExprConstant.cpp
>>>    cfe/trunk/lib/Sema/SemaStmt.cpp
>>>
>>> Modified: cfe/trunk/include/clang/AST/Expr.h
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=172016&r1=172015&r2=172016&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/AST/Expr.h (original)
>>> +++ cfe/trunk/include/clang/AST/Expr.h Wed Jan  9 17:04:56 2013
>>> @@ -568,7 +568,8 @@
>>>   /// EvaluateKnownConstInt - Call EvaluateAsRValue and return the folded
>>>   /// integer. This must be called on an expression that constant folds to an
>>>   /// integer.
>>> -  llvm::APSInt EvaluateKnownConstInt(const ASTContext &Ctx) const;
>>> +  llvm::APSInt EvaluateKnownConstInt(const ASTContext &Ctx,
>>> +                  llvm::SmallVectorImpl<PartialDiagnosticAt> *Diag=0) const;
>>>
>>>   /// EvaluateAsLValue - Evaluate an expression to see if we can fold it to an
>>>   /// lvalue with link time known address, with no side-effects.
>>>
>>> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=172016&r1=172015&r2=172016&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
>>> +++ cfe/trunk/lib/AST/ExprConstant.cpp Wed Jan  9 17:04:56 2013
>>> @@ -6364,8 +6364,10 @@
>>>   return EvaluateAsRValue(Result, Ctx) && !Result.HasSideEffects;
>>> }
>>>
>>> -APSInt Expr::EvaluateKnownConstInt(const ASTContext &Ctx) const {
>>> +APSInt Expr::EvaluateKnownConstInt(const ASTContext &Ctx,
>>> +               llvm::SmallVectorImpl<PartialDiagnosticAt> *Diag) const {
>>>   EvalResult EvalResult;
>>> +  EvalResult.Diag = Diag;
>>>   bool Result = EvaluateAsRValue(EvalResult, Ctx);
>>>   (void)Result;
>>>   assert(Result && "Could not evaluate expression");
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaStmt.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaStmt.cpp?rev=172016&r1=172015&r2=172016&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaStmt.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaStmt.cpp Wed Jan  9 17:04:56 2013
>>> @@ -13,6 +13,7 @@
>>>
>>> #include "clang/Sema/SemaInternal.h"
>>> #include "clang/AST/ASTContext.h"
>>> +#include "clang/AST/ASTDiagnostic.h"
>>> #include "clang/AST/CharUnits.h"
>>> #include "clang/AST/DeclObjC.h"
>>> #include "clang/AST/EvaluatedExprVisitor.h"
>>> @@ -725,7 +726,14 @@
>>>       } else {
>>>         // We already verified that the expression has a i-c-e value (C99
>>>         // 6.8.4.2p3) - get that value now.
>>> -        LoVal = Lo->EvaluateKnownConstInt(Context);
>>> +        llvm::SmallVector<PartialDiagnosticAt, 8> Diags;
>>> +        LoVal = Lo->EvaluateKnownConstInt(Context, &Diags);
>>> +        if (Diags.size() == 1 &&
>>> +            Diags[0].second.getDiagID() == diag::note_constexpr_overflow) {
>>
>> It seems a bit strange to only diagnose in the presence of a specific
>> diagnostic. What's the goal here?
>
> I want to issue warning when case value overflows. EvaluateKnownConstInt detects
> this by returning a note and callers are expected to issue the warning based on the note returned.
> Similar case is handled at end of CheckConvertedConstantExpression.

Well, in CheckConvertedConstantExpression, a diagnostic is produced
any time the evaluation produces notes. In this case, we only issue a
diagnostic for one of the notes. I'm concerned that this code would be
broken if we split that note into multiple notes with more details for
some cases -- and, in fact, there already is such a separate note for
signed left shift overflow.

Have you thought about always warning when an expression contains an
overflowing calculation, not only when such an overflow appears in a
case expression (like we already do for left shifts, for instance)?

> Also, I changed the warning per your other comments.
> In r172102.

Thanks! The "results in a new value" wording seems a bit awkward, how
about just saying "overflow in case constant expression results in
value %0"?