From 4de9091c6235bdb080cee9c79592c3f0f5ef0fcc Mon Sep 17 00:00:00 2001
From: Noel Grandin <noel.grandin@collabora.co.uk>
Date: Mon, 13 Nov 2017 16:13:16 +0200
Subject: [PATCH] loplugin:flatten loosen condition

the description in the comment was right, but the code was not

Change-Id: I7c038e7453f4387d33ec6423c0c55446d6d0df47
Reviewed-on: https://gerrit.libreoffice.org/44680
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
---
 compilerplugins/clang/flatten.cxx      | 17 ++++++++++++-----
 compilerplugins/clang/test/flatten.cxx | 18 ++++++++++++++----
 jvmfwk/source/elements.cxx             |  5 +----
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/compilerplugins/clang/flatten.cxx b/compilerplugins/clang/flatten.cxx
index 7dc2bf019738..fc0e63438368 100644
--- a/compilerplugins/clang/flatten.cxx
+++ b/compilerplugins/clang/flatten.cxx
@@ -45,7 +45,7 @@ private:
     std::string invertCondition(Expr const * condExpr, SourceRange conditionRange);
     bool checkOverlap(SourceRange);
 
-    std::stack<bool> mLastStmtInParentStack;
+    Stmt const * lastStmtInCompoundStmt = nullptr;
     std::vector<std::pair<char const *, char const *>> mvModifiedRanges;
     Stmt const * mElseBranch = nullptr;
 };
@@ -107,9 +107,16 @@ bool Flatten::TraverseCompoundStmt(CompoundStmt * compoundStmt)
     // var decls in its then block, we cannot de-indent the then block without
     // extending the lifetime of some variables, which may be problematic
     // ignore if we are part of an if/then/else/if chain
-    mLastStmtInParentStack.push(compoundStmt->size() > 0 && isa<IfStmt>(*compoundStmt->body_back()));
+    auto copy = lastStmtInCompoundStmt;
+    if (compoundStmt->size() > 0)
+        lastStmtInCompoundStmt = compoundStmt->body_back();
+    else
+        lastStmtInCompoundStmt = nullptr;
+
     bool rv = RecursiveASTVisitor<Flatten>::TraverseCompoundStmt(compoundStmt);
-    mLastStmtInParentStack.pop();
+
+    lastStmtInCompoundStmt = copy;
+    return rv;
     return rv;
 }
 
@@ -142,7 +149,7 @@ bool Flatten::VisitIfStmt(IfStmt const * ifStmt)
         // if the "if" statement is not the last statement in its block, and it contains
         // var decls in its then block, we cannot de-indent the then block without
         // extending the lifetime of some variables, which may be problematic
-        if (!mLastStmtInParentStack.top() || containsVarDecl(ifStmt->getThen()))
+        if (ifStmt != lastStmtInCompoundStmt && containsVarDecl(ifStmt->getThen()))
             return true;
 
         if (!rewrite1(ifStmt))
@@ -164,7 +171,7 @@ bool Flatten::VisitIfStmt(IfStmt const * ifStmt)
         // if the "if" statement is not the last statement in it's block, and it contains
         // var decls in it's else block, we cannot de-indent the else block without
         // extending the lifetime of some variables, which may be problematic
-        if (!mLastStmtInParentStack.top() || containsVarDecl(ifStmt->getElse()))
+        if (ifStmt != lastStmtInCompoundStmt && containsVarDecl(ifStmt->getElse()))
             return true;
 
         if (!rewrite2(ifStmt))
diff --git a/compilerplugins/clang/test/flatten.cxx b/compilerplugins/clang/test/flatten.cxx
index e7b53519de97..30f5c8f41bd9 100644
--- a/compilerplugins/clang/test/flatten.cxx
+++ b/compilerplugins/clang/test/flatten.cxx
@@ -19,12 +19,17 @@ void top1() {
     } else {
         throw std::exception(); // expected-error {{unconditional throw in else branch, rather invert the condition, throw early, and flatten the normal case [loplugin:flatten]}}
     }
-    // no warning expected
     if (foo() == 1) {
         Class aClass;
         (void)aClass;
     } else {
-        throw std::exception();
+        throw std::exception(); // no warning expected
+    }
+    if (foo() == 1) { // expected-note {{if condition here [loplugin:flatten]}}
+        Class aClass;
+        (void)aClass;
+    } else {
+        throw std::exception(); // expected-error {{unconditional throw in else branch, rather invert the condition, throw early, and flatten the normal case [loplugin:flatten]}}
     }
 }
 
@@ -34,9 +39,14 @@ void top2() {
     } else {
         foo();
     }
-    // no warning expected
     if (foo() == 2) {
-        throw std::exception();
+        throw std::exception(); // no warning expected
+    } else {
+        Class aClass;
+        (void)aClass;
+    }
+    if (foo() == 2) {
+        throw std::exception(); // expected-error {{unconditional throw in then branch, just flatten the else [loplugin:flatten]}}
     } else {
         Class aClass;
         (void)aClass;
diff --git a/jvmfwk/source/elements.cxx b/jvmfwk/source/elements.cxx
index 96ef388933d4..ff083373f553 100644
--- a/jvmfwk/source/elements.cxx
+++ b/jvmfwk/source/elements.cxx
@@ -74,10 +74,7 @@ OString getElement(OString const & docPath,
             JFW_E_ERROR,
             "[Java framework] Error in function getElement (elements.cxx)");
     }
-    else
-    {
-        sValue = reinterpret_cast<sal_Char*>(pathObj->nodesetval->nodeTab[0]->content);
-    }
+    sValue = reinterpret_cast<sal_Char*>(pathObj->nodesetval->nodeTab[0]->content);
     return sValue;
 }