[Libreoffice-commits] core.git: compilerplugins/clang

[Stephan Bergmann]

 compilerplugins/clang/flatten.cxx      |   23 +++++++++++++++++++----
 compilerplugins/clang/test/flatten.cxx |   27 ++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 5 deletions(-)

New commits:
commit adf3cf074e05e633e907f72215d56720c01fd2db
Author: Stephan Bergmann <sbergman at redhat.com>
Date:   Tue Nov 7 23:27:54 2017 +0100

    Fix loplugin:flatten's skipping of if/then/else/if chains
    
    (but which finds no new hits)
    
    Change-Id: I5d5f351402797b662a08ec8dca301bd174e22a50
    Reviewed-on: https://gerrit.libreoffice.org/44433
    Tested-by: Jenkins <ci at libreoffice.org>
    Reviewed-by: Stephan Bergmann <sbergman at redhat.com>

diff --git a/compilerplugins/clang/flatten.cxx b/compilerplugins/clang/flatten.cxx
index 7d546ef5b448..7dc2bf019738 100644
--- a/compilerplugins/clang/flatten.cxx
+++ b/compilerplugins/clang/flatten.cxx
@@ -47,6 +47,7 @@ private:
 
     std::stack<bool> mLastStmtInParentStack;
     std::vector<std::pair<char const *, char const *>> mvModifiedRanges;
+    Stmt const * mElseBranch = nullptr;
 };
 
 static Stmt const * containsSingleThrowExpr(Stmt const * stmt)
@@ -84,10 +85,20 @@ bool Flatten::TraverseCXXCatchStmt(CXXCatchStmt* )
 
 bool Flatten::TraverseIfStmt(IfStmt * ifStmt)
 {
-    // ignore if we are part of an if/then/else/if chain
-    if (ifStmt->getElse() && isa<IfStmt>(ifStmt->getElse()))
-        return true;
-    return RecursiveASTVisitor<Flatten>::TraverseIfStmt(ifStmt);
+    if (!WalkUpFromIfStmt(ifStmt)) {
+        return false;
+    }
+    auto const saved = mElseBranch;
+    mElseBranch = ifStmt->getElse();
+    auto ret = true;
+    for (auto const sub: ifStmt->children()) {
+        if (!TraverseStmt(sub)) {
+            ret = false;
+            break;
+        }
+    }
+    mElseBranch = saved;
+    return ret;
 }
 
 bool Flatten::TraverseCompoundStmt(CompoundStmt * compoundStmt)
@@ -110,6 +121,10 @@ bool Flatten::VisitIfStmt(IfStmt const * ifStmt)
     if (!ifStmt->getElse())
         return true;
 
+    // ignore if we are part of an if/then/else/if chain
+    if (ifStmt == mElseBranch || isa<IfStmt>(ifStmt->getElse()))
+        return true;
+
     auto const thenThrowExpr = containsSingleThrowExpr(ifStmt->getThen());
     auto const elseThrowExpr = containsSingleThrowExpr(ifStmt->getElse());
     // If neither contains a throw, nothing to do; if both contain throws, no
diff --git a/compilerplugins/clang/test/flatten.cxx b/compilerplugins/clang/test/flatten.cxx
index dcc7cb3b81c9..e7b53519de97 100644
--- a/compilerplugins/clang/test/flatten.cxx
+++ b/compilerplugins/clang/test/flatten.cxx
@@ -10,7 +10,7 @@
 #include <exception>
 
 extern int foo();
-extern int bar();
+extern int bar(int = 0);
 class Class {};
 
 void top1() {
@@ -104,4 +104,29 @@ void top6() {
     (void)x;
 }
 
+void top7() {
+    // no warning expected
+    if (foo() == 1) {
+        throw std::exception();
+    } else if (foo() == 2) {
+        throw std::exception();
+    } else {
+        throw std::exception();
+    }
+}
+
+void top8() {
+    if (foo() == 1) {
+        if (foo() == 2) {
+            throw std::exception(); // expected-error {{unconditional throw in then branch, just flatten the else [loplugin:flatten]}}
+        } else {
+            bar();
+        }
+    } else if (foo() == 2) {
+        bar(1);
+    } else {
+        bar(2);
+    }
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */