[-Wunreachable-code] Simplify and broad -Wunreachable-code-return, including nontrivi...

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 08ee20930f161ac771b0ef6f11f531003df1245c..ed0d711aefb569e5db1395e74d7524fc60e45e71 100644 (file)
--- 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 a4792d41e8daac78b830d3500b92d94c100110ec..7bc66be8958bd55cd3776cc5de0d601859021a6d 100644 (file)
--- 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<UnreachableCode>, DefaultIgnore;
 def warn_unreachable_break : Warning<
   "'break' will never be executed">,

diff --git a/lib/Analysis/ReachableCode.cpp b/lib/Analysis/ReachableCode.cpp
index 463883f82659d821e43828d72a07b55809d4c046..c79a94b820c5364852fa863a2c976760b0dfc9ca 100644 (file)
--- 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<EnumConstantDecl>(DR->getDecl());
 }
 
-static const Expr *stripStdStringCtor(const Expr *Ex) {
-  // Go crazy pattern matching an implicit construction of std::string("").
-  const ExprWithCleanups *EWC = dyn_cast<ExprWithCleanups>(Ex);
-  if (!EWC)
-    return 0;
-  const CXXConstructExpr *CCE = dyn_cast<CXXConstructExpr>(EWC->getSubExpr());
-  if (!CCE)
-    return 0;
-  QualType Ty = CCE->getType();
-  if (const ElaboratedType *ET = dyn_cast<ElaboratedType>(Ty))
-    Ty = ET->getNamedType();
-  const TypedefType *TT = dyn_cast<TypedefType>(Ty);
-  StringRef Name = TT->getDecl()->getName();
-  if (Name != "string")
-    return 0;
-  if (CCE->getNumArgs() != 1)
-    return 0;
-  const MaterializeTemporaryExpr *MTE =
-    dyn_cast<MaterializeTemporaryExpr>(CCE->getArg(0));
-  if (!MTE)
-    return 0;
-  CXXBindTemporaryExpr *CBT =
-    dyn_cast<CXXBindTemporaryExpr>(MTE->GetTemporaryExpr()->IgnoreParenCasts());
-  if (!CBT)
-    return 0;
-  Ex = CBT->getSubExpr()->IgnoreParenCasts();
-  CCE = dyn_cast<CXXConstructExpr>(Ex);
-  if (!CCE)
-    return 0;
-  if (CCE->getNumArgs() != 1)
-    return 0;
-  return dyn_cast<StringLiteral>(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<IntegerLiteral>(Ex) || isa<StringLiteral>(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<CFGStmt> CS = I->getAs<CFGStmt>()) {
       if (const ReturnStmt *RS = dyn_cast<ReturnStmt>(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<Expr*>(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<BreakStmt>(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<BreakStmt>(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 32e40ab2ea9cb7186a82136e96c7a3913d166395..389109a11bc3a9dd1777ba27af44ed888d576b6f 100644 (file)
--- 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 2370fe0da24a23ad8b540c8ebad0d963043f1cbe..abab966c6603299788c06dfe88faa311de01e5a0 100644 (file)
--- 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}}
+}
+