From: Ted Kremenek Date: Thu, 20 Mar 2014 06:07:30 +0000 (+0000) Subject: [-Wunreachable-code] Simplify and broad -Wunreachable-code-return, including nontrivi... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a80944a18dd23aab4628a38430fcde8f06397a2f;p=clang [-Wunreachable-code] Simplify and broad -Wunreachable-code-return, including nontrivial returns. The exception is return statements that include control-flow, which are clearly doing something "interesting". 99% of the cases I examined for -Wunreachable-code that fired on return statements were not interesting enough to warrant being in -Wunreachable-code by default. Thus the move to include them in -Wunreachable-code-return. This simplifies a bunch of logic, including removing the ad hoc logic to look for std::string literals. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@204307 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/Analyses/ReachableCode.h b/include/clang/Analysis/Analyses/ReachableCode.h index 08ee20930f..ed0d711aef 100644 --- a/include/clang/Analysis/Analyses/ReachableCode.h +++ b/include/clang/Analysis/Analyses/ReachableCode.h @@ -39,7 +39,7 @@ namespace reachable_code { /// Classifications of unreachable code. enum UnreachableKind { - UK_TrivialReturn, + UK_Return, UK_Break, UK_Other }; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index a4792d41e8..7bc66be895 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -362,7 +362,7 @@ def warn_suggest_noreturn_block : Warning< // Unreachable code. def warn_unreachable : Warning< - "will never be executed">, + "code will never be executed">, InGroup, DefaultIgnore; def warn_unreachable_break : Warning< "'break' will never be executed">, diff --git a/lib/Analysis/ReachableCode.cpp b/lib/Analysis/ReachableCode.cpp index 463883f826..c79a94b820 100644 --- a/lib/Analysis/ReachableCode.cpp +++ b/lib/Analysis/ReachableCode.cpp @@ -18,6 +18,7 @@ #include "clang/AST/ExprCXX.h" #include "clang/AST/ExprObjC.h" #include "clang/AST/StmtCXX.h" +#include "clang/AST/ParentMap.h" #include "clang/Analysis/AnalysisContext.h" #include "clang/Analysis/CFG.h" #include "clang/Basic/SourceManager.h" @@ -37,53 +38,6 @@ static bool isEnumConstant(const Expr *Ex) { return isa(DR->getDecl()); } -static const Expr *stripStdStringCtor(const Expr *Ex) { - // Go crazy pattern matching an implicit construction of std::string(""). - const ExprWithCleanups *EWC = dyn_cast(Ex); - if (!EWC) - return 0; - const CXXConstructExpr *CCE = dyn_cast(EWC->getSubExpr()); - if (!CCE) - return 0; - QualType Ty = CCE->getType(); - if (const ElaboratedType *ET = dyn_cast(Ty)) - Ty = ET->getNamedType(); - const TypedefType *TT = dyn_cast(Ty); - StringRef Name = TT->getDecl()->getName(); - if (Name != "string") - return 0; - if (CCE->getNumArgs() != 1) - return 0; - const MaterializeTemporaryExpr *MTE = - dyn_cast(CCE->getArg(0)); - if (!MTE) - return 0; - CXXBindTemporaryExpr *CBT = - dyn_cast(MTE->GetTemporaryExpr()->IgnoreParenCasts()); - if (!CBT) - return 0; - Ex = CBT->getSubExpr()->IgnoreParenCasts(); - CCE = dyn_cast(Ex); - if (!CCE) - return 0; - if (CCE->getNumArgs() != 1) - return 0; - return dyn_cast(CCE->getArg(0)->IgnoreParenCasts()); -} - -/// Strip away "sugar" around trivial expressions that are for the -/// purpose of this analysis considered uninteresting for dead code warnings. -static const Expr *stripExprSugar(const Expr *Ex) { - Ex = Ex->IgnoreParenCasts(); - // If 'Ex' is a constructor for a std::string, strip that - // away. We can only get here if the trivial expression was - // something like a C string literal, with the std::string - // just wrapping that value. - if (const Expr *StdStringVal = stripStdStringCtor(Ex)) - return StdStringVal; - return Ex; -} - static bool isTrivialExpression(const Expr *Ex) { Ex = Ex->IgnoreParenCasts(); return isa(Ex) || isa(Ex) || @@ -104,7 +58,7 @@ static bool isTrivialDoWhile(const CFGBlock *B, const Stmt *S) { return false; } -static bool isTrivialReturn(const CFGBlock *B, const Stmt *S) { +static bool isDeadReturn(const CFGBlock *B, const Stmt *S) { // Look to see if the block ends with a 'return', and see if 'S' // is a substatement. The 'return' may not be the last element in // the block because of destructors. @@ -112,13 +66,19 @@ static bool isTrivialReturn(const CFGBlock *B, const Stmt *S) { I != E; ++I) { if (Optional CS = I->getAs()) { if (const ReturnStmt *RS = dyn_cast(CS->getStmt())) { - // Determine if we need to lock at the body of the block - // before the dead return. if (RS == S) return true; if (const Expr *RE = RS->getRetValue()) { - RE = stripExprSugar(RE->IgnoreParenCasts()); - return RE == S && isTrivialExpression(RE); + RE = RE->IgnoreParenCasts(); + if (RE == S) + return true; + ParentMap PM(const_cast(RE)); + // If 'S' is in the ParentMap, it is a subexpression of + // the return statement. Note also that we are restricting + // to looking at return statements in the same CFGBlock, + // so this will intentionally not catch cases where the + // return statement contains nested control-flow. + return PM.getParent(S); } } break; @@ -547,28 +507,18 @@ static SourceLocation GetUnreachableLoc(const Stmt *S, void DeadCodeScan::reportDeadCode(const CFGBlock *B, const Stmt *S, clang::reachable_code::Callback &CB) { - // The kind of unreachable code found. + // Classify the unreachable code found, or suppress it in some cases. reachable_code::UnreachableKind UK = reachable_code::UK_Other; - do { - // Suppress idiomatic cases of calling a noreturn function just - // before executing a 'break'. If there is other code after the 'break' - // in the block then don't suppress the warning. - if (isa(S)) { - UK = reachable_code::UK_Break; - break; - } - - if (isTrivialDoWhile(B, S)) - return; - - // Suppress trivial 'return' statements that are dead. - if (isTrivialReturn(B, S)) { - UK = reachable_code::UK_TrivialReturn; - break; - } - - } while(false); + if (isa(S)) { + UK = reachable_code::UK_Break; + } + else if (isTrivialDoWhile(B, S)) { + return; + } + else if (isDeadReturn(B, S)) { + UK = reachable_code::UK_Return; + } SourceRange R1, R2; SourceLocation Loc = GetUnreachableLoc(S, R1, R2); diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index 32e40ab2ea..389109a11b 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -73,7 +73,7 @@ namespace { case reachable_code::UK_Break: diag = diag::warn_unreachable_break; break; - case reachable_code::UK_TrivialReturn: + case reachable_code::UK_Return: diag = diag::warn_unreachable_return; break; case reachable_code::UK_Other: diff --git a/test/SemaCXX/warn-unreachable.cpp b/test/SemaCXX/warn-unreachable.cpp index 2370fe0da2..abab966c66 100644 --- a/test/SemaCXX/warn-unreachable.cpp +++ b/test/SemaCXX/warn-unreachable.cpp @@ -218,3 +218,19 @@ int test_treat_non_const_bool_local_as_non_config_value() { return 0; } +class Frobozz { +public: + Frobozz(int x); + ~Frobozz(); +}; + +Frobozz test_return_object(int flag) { + return Frobozz(flag); + return Frobozz(42); // expected-warning {{'return' will never be executed}} +} + +Frobozz test_return_object_control_flow(int flag) { + return Frobozz(flag); + return Frobozz(flag ? 42 : 24); // expected-warning {{code will never be executed}} +} +