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

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