[cfe-commits] r172016 - in /cfe/trunk: include/clang/AST/Expr.h lib/AST/ExprConstant.cpp lib/Sema/SemaStmt.cpp test/Sema/switch-1.c
jahanian
fjahanian at apple.com
Thu Jan 10 12:31:42 PST 2013
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.
Also, I changed the warning per your other comments.
In r172102.
- Thanks, Fariborz
>
>> + Diag(Lo->getLocStart(), diag::warn_case_value_overflow) <<
>> + LoVal.toString(10) << "switch condition value";
>
> Please do not use literal English-language strings as arguments to
> diagnostics. Either switch this diagnostic to use a %select, or add a
> separate diagnostic for this case. (You also have no test coverage for
> this string being appropriately incorporated into the diagnostic.)
>
>> + Diag(Diags[0].first, Diags[0].second);
>> + }
>>
>> // If the LHS is not the same type as the condition, insert an implicit
>> // cast.
>>
>> Added: cfe/trunk/test/Sema/switch-1.c
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/switch-1.c?rev=172016&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/Sema/switch-1.c (added)
>> +++ cfe/trunk/test/Sema/switch-1.c Wed Jan 9 17:04:56 2013
>> @@ -0,0 +1,17 @@
>> +// RUN: %clang_cc1 -fsyntax-only -verify -triple x86_64-apple-darwin10 %s
>> +// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -triple x86_64-apple-darwin10 %s
>> +// rdar://11577384
>> +
>> +int f(int i) {
>> + switch (i) {
>> + case 2147483647 + 2: // expected-note {{value 2147483649 is outside the range of representable values of type 'int'}} \
>> + // expected-warning {{overflow converting case value to switch condition type}}
>
> This diagnostic doesn't look correct: the overflow didn't occur while
> converting the case value.
>
>> + return 1;
>> + case 9223372036854775807L * 4 : // expected-note {{value 36893488147419103228 is outside the range of representable values of type 'long'}} \
>> + // expected-warning {{overflow converting case value to switch condition type}}
>> + return 2;
>> + case 2147483647:
>> + return 0;
>> + }
>> + return 0;
>> +}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list