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

[Matt Beaumont-Gay]

On Thu, Jan 10, 2013 at 6:39 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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)?

Feature request for general -Woverflow is PR2729. (I think PR14043
might also be a dup.) As several bug reports have pointed out, GCC has
such a warning.

-Matt