new loplugin collapseif

Look for nested if statements with relatively small conditions, where
they can be collapsed into one if statement.

Change-Id: I7d5d4e418d0ce928991a3308fc88969c00c0d0f2
Reviewed-on: https://gerrit.libreoffice.org/62898
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

3 files changed, 185 insertions, 0 deletions

diff --git a/compilerplugins/clang/collapseif.cxx b/compilerplugins/clang/collapseif.cxx
new file mode 100644
index 000000000000..b3f192c2ce1d
--- /dev/null
+++ b/compilerplugins/clang/collapseif.cxx
@@ -0,0 +1,129 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <cassert>
+#include <string>
+#include <iostream>
+#include <unordered_map>
+#include <unordered_set>
+
+#include "plugin.hxx"
+#include "check.hxx"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/StmtVisitor.h"
+
+/**
+    Look for nested if statements with relatively small conditions, where they can be collapsed into
+    one if statement.
+*/
+namespace
+{
+class CollapseIf : public loplugin::FilteringPlugin<CollapseIf>
+{
+public:
+    explicit CollapseIf(loplugin::InstantiationData const& data)
+        : FilteringPlugin(data)
+    {
+    }
+
+    virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
+
+    bool VisitIfStmt(IfStmt const*);
+
+private:
+    int getNoCharsInSourceCodeOfExpr(IfStmt const*);
+    bool containsComment(Stmt const* stmt);
+};
+
+bool CollapseIf::VisitIfStmt(IfStmt const* ifStmt)
+{
+    if (ignoreLocation(ifStmt))
+        return true;
+    if (ifStmt->getElse())
+        return true;
+
+    IfStmt const* secondIfStmt = nullptr;
+    if (auto compoundStmt = dyn_cast<CompoundStmt>(ifStmt->getThen()))
+    {
+        if (compoundStmt->size() != 1)
+            return true;
+        secondIfStmt = dyn_cast<IfStmt>(*compoundStmt->body_begin());
+        if (!secondIfStmt)
+            return true;
+        if (secondIfStmt->getElse())
+            return true;
+    }
+    else
+    {
+        secondIfStmt = dyn_cast<IfStmt>(ifStmt->getThen());
+        if (!secondIfStmt)
+            return true;
+    }
+
+    int noChars1 = getNoCharsInSourceCodeOfExpr(ifStmt);
+    int noChars2 = getNoCharsInSourceCodeOfExpr(secondIfStmt);
+    if (noChars1 + noChars2 > 40)
+        return true;
+
+    // Sometimes there is a comment between the first and second if, so
+    // merging them would make the comment more awkward to write.
+    if (containsComment(ifStmt))
+        return true;
+
+    report(DiagnosticsEngine::Warning, "nested if should be collapsed into one statement %0 %1",
+           compat::getBeginLoc(ifStmt))
+        << noChars1 << noChars2 << ifStmt->getSourceRange();
+    return true;
+}
+
+int CollapseIf::getNoCharsInSourceCodeOfExpr(IfStmt const* ifStmt)
+{
+    // Measure the space between the "if" the beginning of the "then" block because
+    // measuring the size of the condition expression is unreliable, because clang
+    // does not report the location of the last token accurately.
+    SourceManager& SM = compiler.getSourceManager();
+    SourceLocation startLoc = compat::getBeginLoc(ifStmt);
+    SourceLocation endLoc = compat::getBeginLoc(ifStmt->getThen());
+    char const* p1 = SM.getCharacterData(startLoc);
+    char const* p2 = SM.getCharacterData(endLoc);
+
+    int count = 0;
+    for (auto p = p1; p < p2; ++p)
+        if (*p != ' ')
+            ++count;
+
+    return count;
+}
+
+bool CollapseIf::containsComment(Stmt const* stmt)
+{
+    SourceManager& SM = compiler.getSourceManager();
+    auto range = stmt->getSourceRange();
+    SourceLocation startLoc = range.getBegin();
+    SourceLocation endLoc = range.getEnd();
+    char const* p1 = SM.getCharacterData(startLoc);
+    char const* p2 = SM.getCharacterData(endLoc);
+    p2 += Lexer::MeasureTokenLength(endLoc, SM, compiler.getLangOpts());
+
+    // check for comments
+    constexpr char const comment1[] = "/*";
+    constexpr char const comment2[] = "//";
+    if (std::search(p1, p2, comment1, comment1 + strlen(comment1)) != p2)
+        return true;
+    if (std::search(p1, p2, comment2, comment2 + strlen(comment2)) != p2)
+        return true;
+
+    return false;
+}
+
+/** Off by default because some places are a judgement call if it should be collapsed or not. */
+loplugin::Plugin::Registration<CollapseIf> X("collapseif", false);
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/test/collapseif.cxx b/compilerplugins/clang/test/collapseif.cxx
new file mode 100644
index 000000000000..6f8658473918
--- /dev/null
+++ b/compilerplugins/clang/test/collapseif.cxx
@@ -0,0 +1,55 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <sal/config.h>
+
+int f1(int x)
+{
+    // expected-error@+1 {{nested if should be collapsed into one statement 9 9 [loplugin:collapseif]}}
+    if (x == 1)
+        if (x == 2)
+            return 1;
+
+    // expected-error@+1 {{nested if should be collapsed into one statement 9 9 [loplugin:collapseif]}}
+    if (x == 1)
+    {
+        if (x == 2)
+            return 1;
+    }
+
+    // no warning expected
+    if (x == 1)
+    {
+        // comment here prevents warning
+        if (x == 2)
+            return 1;
+    }
+
+    // no warning expected
+    if (x == 1)
+    {
+        if (x == 2)
+            return 1;
+    }
+    else
+        return 3;
+
+    // no warning expected
+    if (x == 1)
+    {
+        if (x == 2)
+            return 1;
+        else
+            return 3;
+    }
+
+    return 2;
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk
index 0c086df0f7a4..a4d46ced20a8 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -13,6 +13,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
     compilerplugins/clang/test/badstatics \
     compilerplugins/clang/test/blockblock \
     compilerplugins/clang/test/casttovoid \
+    compilerplugins/clang/test/collapseif \
     compilerplugins/clang/test/commaoperator \
     $(if $(filter-out WNT,$(OS)),compilerplugins/clang/test/constfields) \
     compilerplugins/clang/test/constparams \