New check: Unexpected flag enumerator value

13 files changed, 257 insertions, 0 deletions

diff --git a/Changelog b/Changelog
index 103b34c1..d6dcf380 100644
--- a/Changelog
+++ b/Changelog
@@ -4,6 +4,7 @@
     - New Checks:
       - use-arrow-operator-instead-of-data
       - use-static-qregularexpression
+      - unexpected-flag-enumerator-value
 
 * v1.10 (July 20th 2021)
    - Requires C++17
diff --git a/CheckSources.cmake b/CheckSources.cmake
index 1b4b3c8a..8de99923 100644
--- a/CheckSources.cmake
+++ b/CheckSources.cmake
@@ -24,6 +24,7 @@ set(CLAZY_CHECKS_SRCS ${CLAZY_CHECKS_SRCS}
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/signal-with-return-value.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/thread-with-slots.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/tr-non-literal.cpp
+  ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/unexpected-flag-enumerator-value.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/unneeded-cast.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/use-arrow-operator-instead-of-data.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/use-chrono-in-qtimer.cpp
diff --git a/ClazyTests.cmake b/ClazyTests.cmake
index f942082c..92946b1a 100644
--- a/ClazyTests.cmake
+++ b/ClazyTests.cmake
@@ -25,6 +25,7 @@ add_test(NAME reserve-candidates COMMAND python3 run_tests.py reserve-candidates
 add_test(NAME signal-with-return-value COMMAND python3 run_tests.py signal-with-return-value WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests/)
 add_test(NAME thread-with-slots COMMAND python3 run_tests.py thread-with-slots WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests/)
 add_test(NAME tr-non-literal COMMAND python3 run_tests.py tr-non-literal WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests/)
+add_test(NAME unexpected-flag-enumerator-value COMMAND python3 run_tests.py unexpected-flag-enumerator-value WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests/)
 add_test(NAME unneeded-cast COMMAND python3 run_tests.py unneeded-cast WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests/)
 add_test(NAME use-arrow-operator-instead-of-data COMMAND python3 run_tests.py use-arrow-operator-instead-of-data WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests/)
 add_test(NAME use-chrono-in-qtimer COMMAND python3 run_tests.py use-chrono-in-qtimer WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests/)
diff --git a/README.md b/README.md
index c78c16ff..caa4954a 100644
--- a/README.md
+++ b/README.md
@@ -247,6 +247,7 @@ clazy runs all checks from level1 by default.
     - [signal-with-return-value](docs/checks/README-signal-with-return-value.md)
     - [thread-with-slots](docs/checks/README-thread-with-slots.md)
     - [tr-non-literal](docs/checks/README-tr-non-literal.md)
+    - [unexpected-flag-enumerator-value](docs/checks/README-unexpected-flag-enumerator-value.md)
     - [unneeded-cast](docs/checks/README-unneeded-cast.md)
     - [use-arrow-operator-instead-of-data](docs/checks/README-use-arrow-operator-instead-of-data.md)
     - [use-chrono-in-qtimer](docs/checks/README-use-chrono-in-qtimer.md)
diff --git a/checks.json b/checks.json
index 1faba11e..a1afaab4 100644
--- a/checks.json
+++ b/checks.json
@@ -725,6 +725,13 @@
             "level" : 0,
             "categories" : ["performance"],
             "visits_stmts" : true
+        },
+        {
+            "name"  : "unexpected-flag-enumerator-value",
+            "class_name" : "UnexpectedFlagEnumeratorValue",
+            "level" : -1,
+            "categories" : ["bug"],
+            "visits_decls" : true
         }
     ]
 }
diff --git a/docs/checks/README-unexpected-flag-enumerator-value.md b/docs/checks/README-unexpected-flag-enumerator-value.md
new file mode 100644
index 00000000..ddf832d2
--- /dev/null
+++ b/docs/checks/README-unexpected-flag-enumerator-value.md
@@ -0,0 +1,14 @@
+# unexpected-flag-enumerator-value
+
+Checks `enum`s that are used as flag, for example:
+
+```cpp
+enum Foo {
+    A = 0x1,
+    B = 0x2,
+    C = 0x4,
+    D = 0x9 // Oops, typo
+};
+```
+
+and checks whether all enumeration values are power of 2 or not.
diff --git a/readmes.cmake b/readmes.cmake
index 97e69cbc..8c21ba32 100644
--- a/readmes.cmake
+++ b/readmes.cmake
@@ -24,6 +24,7 @@ SET(README_manuallevel_FILES
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-signal-with-return-value.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-thread-with-slots.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-tr-non-literal.md
+    ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-unexpected-flag-enumerator-value.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-unneeded-cast.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-use-arrow-operator-instead-of-data.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-use-chrono-in-qtimer.md
diff --git a/src/Checks.h b/src/Checks.h
index cbcce87c..c088433e 100644
--- a/src/Checks.h
+++ b/src/Checks.h
@@ -52,6 +52,7 @@
 #include "checks/manuallevel/signal-with-return-value.h"
 #include "checks/manuallevel/thread-with-slots.h"
 #include "checks/manuallevel/tr-non-literal.h"
+#include "checks/manuallevel/unexpected-flag-enumerator-value.h"
 #include "checks/manuallevel/unneeded-cast.h"
 #include "checks/manuallevel/use-arrow-operator-instead-of-data.h"
 #include "checks/manuallevel/use-chrono-in-qtimer.h"
@@ -164,6 +165,7 @@ void CheckManager::registerChecks()
     registerCheck(check<SignalWithReturnValue>("signal-with-return-value", ManualCheckLevel,  RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<ThreadWithSlots>("thread-with-slots", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts | RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<TrNonLiteral>("tr-non-literal", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
+    registerCheck(check<UnexpectedFlagEnumeratorValue>("unexpected-flag-enumerator-value", ManualCheckLevel,  RegisteredCheck::Option_VisitsDecls));
     registerCheck(check<UnneededCast>("unneeded-cast", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
     registerCheck(check<UseArrowOperatorInsteadOfData>("use-arrow-operator-instead-of-data", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
     registerCheck(check<UseChronoInQTimer>("use-chrono-in-qtimer", ManualCheckLevel,  RegisteredCheck::Option_VisitsStmts));
diff --git a/src/checks/manuallevel/unexpected-flag-enumerator-value.cpp b/src/checks/manuallevel/unexpected-flag-enumerator-value.cpp
new file mode 100644
index 00000000..0c918647
--- /dev/null
+++ b/src/checks/manuallevel/unexpected-flag-enumerator-value.cpp
@@ -0,0 +1,132 @@
+/*
+  This file is part of the clazy static checker.
+
+  Copyright (C) 2021 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com
+  Author: Waqar Ahmed <waqar.ahmed@kdab.com>
+
+  This library is free software; you can redistribute it and/or
+  modify it under the terms of the GNU Library General Public
+  License as published by the Free Software Foundation; either
+  version 2 of the License, or (at your option) any later version.
+
+  This library is distributed in the hope that it will be useful,
+  but WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+  Library General Public License for more details.
+
+  You should have received a copy of the GNU Library General Public License
+  along with this library; see the file COPYING.LIB.  If not, write to
+  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+  Boston, MA 02110-1301, USA.
+*/
+
+#include "unexpected-flag-enumerator-value.h"
+#include "Utils.h"
+#include "HierarchyUtils.h"
+#include "QtUtils.h"
+#include "TypeUtils.h"
+
+#include <clang/AST/AST.h>
+
+using namespace clang;
+using namespace std;
+
+
+UnexpectedFlagEnumeratorValue::UnexpectedFlagEnumeratorValue(const std::string &name, ClazyContext *context)
+    : CheckBase(name, context)
+{
+}
+
+static bool isIntentionallyNotPowerOf2(EnumConstantDecl *en) {
+
+    ConstantExpr *cexpr = dyn_cast_or_null<ConstantExpr>(clazy::getFirstChild(en->getInitExpr()));
+    if (!cexpr) {
+        return false;
+    }
+
+    if (auto binaryOp = dyn_cast_or_null<BinaryOperator>(cexpr->getSubExpr())) {
+        binaryOp->dump();
+        return true;
+    }
+
+    return false;
+}
+
+struct IsFlagEnumResult {
+    bool isFlagEnum;
+    int numFalseValues;
+};
+
+static SmallVector<EnumConstantDecl*, 16> getEnumerators(EnumDecl *enDecl)
+{
+    SmallVector<EnumConstantDecl*, 16> ret;
+    for (auto *enumerator : enDecl->enumerators()) {
+        ret.push_back(enumerator);
+    }
+    return ret;
+}
+
+static uint64_t getIntegerValue(EnumConstantDecl* e)
+{
+    return e->getInitVal().getLimitedValue();
+}
+
+static bool hasConsecutiveValues(const SmallVector<EnumConstantDecl*, 16>& enumerators)
+{
+    auto val = getIntegerValue(enumerators.front());
+    bool consecutive = true;
+    for (size_t i = 1; i < enumerators.size(); ++i) {
+        val++;
+        consecutive = getIntegerValue(enumerators[i]) == val;
+    }
+    return consecutive;
+}
+
+static IsFlagEnumResult isFlagEnum(const SmallVector<EnumConstantDecl*, 16>& enumerators)
+{
+    if (enumerators.size() < 4) {
+        return {false, 0};
+    }
+
+    if (hasConsecutiveValues(enumerators)) {
+        return {false, 0};
+    }
+
+    llvm::SmallVector<bool, 16> enumValues;
+    for (auto *enumerator : enumerators) {
+        enumValues.push_back(enumerator->getInitVal().isPowerOf2());
+    }
+
+    const size_t count = std::count(enumValues.begin(), enumValues.end(), false);
+
+    // If half of our values were power-of-2, this is probably a flag enum
+    IsFlagEnumResult res;
+    res.isFlagEnum = count <= (enumerators.size() / 2);
+    res.numFalseValues = count;
+    return res;
+}
+
+void UnexpectedFlagEnumeratorValue::VisitDecl(clang::Decl *decl)
+{
+    EnumDecl* enDecl = dyn_cast_or_null<EnumDecl>(decl);
+    if (!enDecl) {
+        return;
+    }
+
+    const SmallVector<EnumConstantDecl*, 16> enumerators = getEnumerators(enDecl);
+
+    auto flagEnum = isFlagEnum(enumerators);
+    if (!flagEnum.isFlagEnum) {
+        return;
+    }
+
+    for (EnumConstantDecl* enumerator : enumerators) {
+        if (!enumerator->getInitVal().isPowerOf2()) {
+            if (isIntentionallyNotPowerOf2(enumerator)) {
+                continue;
+            }
+            const auto value = enumerator->getInitVal().getLimitedValue();
+            emitWarning(enumerator->getInitExpr()->getBeginLoc(), "Unexpected non power-of-2 enumerator value: " + std::to_string(value));
+        }
+    }
+}
diff --git a/src/checks/manuallevel/unexpected-flag-enumerator-value.h b/src/checks/manuallevel/unexpected-flag-enumerator-value.h
new file mode 100644
index 00000000..cd1b770f
--- /dev/null
+++ b/src/checks/manuallevel/unexpected-flag-enumerator-value.h
@@ -0,0 +1,40 @@
+/*
+  This file is part of the clazy static checker.
+
+  Copyright (C) 2021 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com
+  Author: Waqar Ahmed <waqar.ahmed@kdab.com>
+
+  This library is free software; you can redistribute it and/or
+  modify it under the terms of the GNU Library General Public
+  License as published by the Free Software Foundation; either
+  version 2 of the License, or (at your option) any later version.
+
+  This library is distributed in the hope that it will be useful,
+  but WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+  Library General Public License for more details.
+
+  You should have received a copy of the GNU Library General Public License
+  along with this library; see the file COPYING.LIB.  If not, write to
+  the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+  Boston, MA 02110-1301, USA.
+*/
+
+#ifndef CLAZY_UNEXPECTED_FLAG_ENUMERATOR_VALUE_H
+#define CLAZY_UNEXPECTED_FLAG_ENUMERATOR_VALUE_H
+
+#include "checkbase.h"
+
+
+/**
+ * See README-unexpected-flag-enumerator-value.md for more info.
+ */
+class UnexpectedFlagEnumeratorValue : public CheckBase
+{
+public:
+    explicit UnexpectedFlagEnumeratorValue(const std::string &name, ClazyContext *context);
+    void VisitDecl(clang::Decl *) override;
+private:
+};
+
+#endif
diff --git a/tests/unexpected-flag-enumerator-value/config.json b/tests/unexpected-flag-enumerator-value/config.json
new file mode 100644
index 00000000..e7e6e0cb
--- /dev/null
+++ b/tests/unexpected-flag-enumerator-value/config.json
@@ -0,0 +1,7 @@
+{
+    "tests" : [
+        {
+            "filename" : "main.cpp"
+        }
+    ]
+}
diff --git a/tests/unexpected-flag-enumerator-value/main.cpp b/tests/unexpected-flag-enumerator-value/main.cpp
new file mode 100644
index 00000000..c3209c36
--- /dev/null
+++ b/tests/unexpected-flag-enumerator-value/main.cpp
@@ -0,0 +1,48 @@
+
+// Enum is too small, won't be checked
+enum TooSmall {
+    FooTooSmall = 1,            // Ok
+    BarTooSmall = 2,            // Ok
+    LalaTooSmall = 4,            // Ok
+};
+
+
+enum TEST_FLAGS {
+    Foo = 1,            // Ok
+    Bar = 2,            // Ok
+    Both = Foo | Bar,   // OK
+    Something = 3       // Warn
+};
+
+// Copied from Qt for testing purposes
+enum AlignmentFlag {
+    AlignLeft = 0x0001,
+    AlignLeading = AlignLeft,
+    AlignRight = 0x0002,
+    AlignTrailing = AlignRight,
+    AlignHCenter = 0x0004,
+    AlignJustify = 0x0008,
+    AlignAbsolute = 0x0011, // Warn
+    AlignHorizontal_Mask = AlignLeft | AlignRight | AlignHCenter | AlignJustify | AlignAbsolute,
+
+    AlignTop = 0x0020,
+    AlignBottom = 0x0040,
+    AlignVCenter = 0x0080,
+    AlignBaseline = 0x0100,
+    // Note that 0x100 will clash with Qt::TextSingleLine = 0x100 due to what the comment above
+    // this enum declaration states. However, since Qt::AlignBaseline is only used by layouts,
+    // it doesn't make sense to pass Qt::AlignBaseline to QPainter::drawText(), so there
+    // shouldn't really be any ambiguity between the two overlapping enum values.
+    AlignVertical_Mask = AlignTop | AlignBottom | AlignVCenter | AlignBaseline,
+
+    AlignCenter = AlignVCenter | AlignHCenter
+};
+
+// No Warning here, all consecutive values
+enum {
+    RED = 1,
+    GREEN = 2,
+    BLUE = 3,
+    BLACK = 4,
+    WHITE = 5,
+};
diff --git a/tests/unexpected-flag-enumerator-value/main.cpp.expected b/tests/unexpected-flag-enumerator-value/main.cpp.expected
new file mode 100644
index 00000000..7d3144a4
--- /dev/null
+++ b/tests/unexpected-flag-enumerator-value/main.cpp.expected
@@ -0,0 +1,2 @@
+unexpected-flag-enumerator-value/main.cpp:14:17: warning: Unexpected non power-of-2 enumerator value: 3 [-Wclazy-unexpected-flag-enumerator-value]
+unexpected-flag-enumerator-value/main.cpp:25:21: warning: Unexpected non power-of-2 enumerator value: 17 [-Wclazy-unexpected-flag-enumerator-value]