new checker for checking that comparison operators use all member variablesupstream/work/member_check

author: Shivam kunwar <shivam.kunwar@kdab.com> 2023-02-12 20:38:37 +0530
committer: Alexander Lohnau <alexander.lohnau@gmx.de> 2025-09-22 21:43:36 +0200
commit: 123116bba09ba001ef752195c0f780f0bfc7953e (patch)
tree: 877121b67485a8c36aafa3f5c4f4aec17cfad37f
parent: 9b3707939322f8ae25b7fdc2f13a453eed9d71ee (diff)
12 files changed, 308 insertions, 0 deletions
diff --git a/CheckSources.generated.cmake b/CheckSources.generated.cmake
index 5cc63065..3df9ade2 100644
--- a/CheckSources.generated.cmake
+++ b/CheckSources.generated.cmake
@@ -4,6 +4,7 @@
 
 set(CLAZY_CHECKS_SRCS ${CLAZY_CHECKS_SRCS}
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/assert-with-side-effects.cpp
+  ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/compare-member-check.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/container-inside-loop.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/detaching-member.cpp
   ${CMAKE_CURRENT_LIST_DIR}/src/checks/manuallevel/heap-allocated-small-trivial-type.cpp
diff --git a/ClazyTests.generated.cmake b/ClazyTests.generated.cmake
index fb5b5bd6..ee0e1ac0 100644
--- a/ClazyTests.generated.cmake
+++ b/ClazyTests.generated.cmake
@@ -3,6 +3,7 @@
 # SPDX-License-Identifier: CC0-1.0
 # SPDX-FileCopyrightText: Clazy Developers
 add_clazy_test(assert-with-side-effects)
+add_clazy_test(compare-member-check)
 add_clazy_test(container-inside-loop)
 add_clazy_test(detaching-member)
 add_clazy_test(heap-allocated-small-trivial-type)
diff --git a/README.md b/README.md
index 254cc720..e5060536 100644
--- a/README.md
+++ b/README.md
@@ -222,6 +222,7 @@ clazy runs all checks from level1 by default.
 - Checks from Manual Level:
 
   - [assert-with-side-effects](docs/checks/README-assert-with-side-effects.md)
+  - [compare-member-check](docs/checks/README-compare-member-check.md)
   - [container-inside-loop](docs/checks/README-container-inside-loop.md)
   - [detaching-member](docs/checks/README-detaching-member.md)
   - [heap-allocated-small-trivial-type](docs/checks/README-heap-allocated-small-trivial-type.md)
diff --git a/checks.json b/checks.json
index c21df7f1..cb156f03 100644
--- a/checks.json
+++ b/checks.json
@@ -787,6 +787,13 @@
             "name": "readlock-detaching",
             "level": 1,
             "categories": ["bug"]
+        },
+        {
+            "name": "compare-member-check",
+            "level": -1,
+            "categories": ["bug"],
+            "visits_stmts": true,
+            "can_ignore_includes": true
         }
     ]
 }
diff --git a/docs/checks/README-compare-member-check.md b/docs/checks/README-compare-member-check.md
new file mode 100644
index 00000000..5a49f116
--- /dev/null
+++ b/docs/checks/README-compare-member-check.md
@@ -0,0 +1,32 @@
+# compare-member-check
+
+Warns when comparison operators don't use all member variables of a class.
+This helps prevent bugs by ensuring consistent comparisons, particularly important
+when using containers or sorting.
+
+Example:
+```cpp
+class ExampleClass {
+public:
+    int a;
+    int b;
+    int c;
+    
+    bool operator>(const ExampleClass &other) const {
+        return a > other.a || b > other.b; // Warning: 'c' is not used
+    }
+    
+    // Fixed using std::tie:
+    bool operator<(const ExampleClass &rhs) const {
+        return std::tie(a, b, c) < std::tie(rhs.a, rhs.b, rhs.c); // OK
+    }
+};
+```
+
+The check will not warn for:
+- Classes without member variables
+- Qt's meta-object system methods
+- Template classes
+
+For implementing comparisons, prefer using `std::tie` as it provides a less error-prone
+way to compare multiple members. See `std::tie` documentation for more information.
diff --git a/readmes.cmake b/readmes.cmake
index 2c191a53..ec82f6df 100644
--- a/readmes.cmake
+++ b/readmes.cmake
@@ -1,5 +1,6 @@
 SET(README_manuallevel_FILES
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-assert-with-side-effects.md
+    ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-compare-member-check.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-container-inside-loop.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-detaching-member.md
     ${CMAKE_CURRENT_LIST_DIR}/docs/checks/README-heap-allocated-small-trivial-type.md
diff --git a/src/Checks.h b/src/Checks.h
index 07da15f0..35e14f3e 100644
--- a/src/Checks.h
+++ b/src/Checks.h
@@ -77,6 +77,7 @@
 #include "checks/level2/static-pmf.h"
 #include "checks/level2/virtual-call-ctor.h"
 #include "checks/manuallevel/assert-with-side-effects.h"
+#include "checks/manuallevel/compare-member-check.h"
 #include "checks/manuallevel/container-inside-loop.h"
 #include "checks/manuallevel/detaching-member.h"
 #include "checks/manuallevel/heap-allocated-small-trivial-type.h"
@@ -117,6 +118,7 @@ RegisteredCheck check(const char *name, CheckLevel level, RegisteredCheck::Optio
 void CheckManager::registerChecks()
 {
     registerCheck(check<AssertWithSideEffects>("assert-with-side-effects", ManualCheckLevel, RegisteredCheck::Option_VisitsStmts, true));
+    registerCheck(check<CompareMemberCheck>("compare-member-check", ManualCheckLevel, RegisteredCheck::Option_VisitsStmts, true));
     registerCheck(check<ContainerInsideLoop>("container-inside-loop", ManualCheckLevel, RegisteredCheck::Option_VisitsStmts, true));
     registerCheck(check<DetachingMember>("detaching-member", ManualCheckLevel, RegisteredCheck::Option_VisitsStmts, true));
     registerCheck(check<HeapAllocatedSmallTrivialType>("heap-allocated-small-trivial-type", ManualCheckLevel, RegisteredCheck::Option_VisitsDecls, false));
diff --git a/src/checks/manuallevel/compare-member-check.cpp b/src/checks/manuallevel/compare-member-check.cpp
new file mode 100644
index 00000000..da969220
--- /dev/null
+++ b/src/checks/manuallevel/compare-member-check.cpp
@@ -0,0 +1,143 @@
+/*
+    This file is part of the clazy static checker.
+
+    Copyright (C) 2023 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com
+    Author: Shivam Kunwar <shivam.kunwar@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 "compare-member-check.h"
+#include <clang/AST/AST.h>
+#include <clang/AST/ExprCXX.h>
+#include <clang/AST/ParentMap.h>
+#include <clang/ASTMatchers/ASTMatchFinder.h>
+#include <clang/ASTMatchers/ASTMatchers.h>
+#include <clang/ASTMatchers/ASTMatchersInternal.h>
+#include <iostream>
+
+using namespace clang::ast_matchers;
+class ClazyContext;
+
+using namespace clang;
+
+void checkBinaryOperator(const BinaryOperator *binOp, const CXXRecordDecl *record, std::vector<const FieldDecl *> &classFields)
+{
+    if (const BinaryOperator *leftBO = llvm::dyn_cast<BinaryOperator>(binOp->getLHS())) {
+        checkBinaryOperator(leftBO, record, classFields);
+    }
+
+    if (const BinaryOperator *rightBO = llvm::dyn_cast<BinaryOperator>(binOp->getRHS())) {
+        checkBinaryOperator(rightBO, record, classFields);
+    }
+
+    const MemberExpr *lhsMemberExpr = nullptr;
+    if (const ImplicitCastExpr *implicitCast = dyn_cast<ImplicitCastExpr>(binOp->getLHS())) {
+        lhsMemberExpr = dyn_cast<MemberExpr>(implicitCast->getSubExpr());
+    } else if (const MemberExpr *temp = dyn_cast<MemberExpr>(binOp->getLHS())) {
+        lhsMemberExpr = temp;
+    }
+
+    if (lhsMemberExpr) {
+        FieldDecl *fieldDecl = dyn_cast<FieldDecl>(lhsMemberExpr->getMemberDecl());
+        if (fieldDecl && fieldDecl->getParent() == record) {
+            auto it = std::find(classFields.begin(), classFields.end(), fieldDecl);
+
+            if (it != classFields.end()) {
+                classFields.erase(it);
+            }
+        }
+    }
+}
+
+class Caller : public ClazyAstMatcherCallback
+{
+public:
+    Caller(CheckBase *check)
+        : ClazyAstMatcherCallback(check)
+    {
+    }
+    virtual void run(const MatchFinder::MatchResult &result)
+    {
+        if (const CXXOperatorCallExpr *callExpr = result.Nodes.getNodeAs<CXXOperatorCallExpr>("callExpr")) {
+            const clang::Decl *calleeDecl = callExpr->getCalleeDecl();
+            if (!calleeDecl)
+                return;
+
+            const auto *methodDecl = dyn_cast<CXXMethodDecl>(calleeDecl);
+            if (!methodDecl)
+                return;
+
+            // Get the class declaration
+            const CXXRecordDecl *classDecl = methodDecl->getParent();
+
+            std::vector<const FieldDecl *> classFields;
+            for (const auto *field : classDecl->fields()) {
+                classFields.push_back(field);
+            }
+            for (const auto *stmt : methodDecl->getBody()->children()) {
+                const ReturnStmt *returnStmt = dyn_cast<ReturnStmt>(stmt);
+                if (!returnStmt)
+                    return;
+                const Expr *returnExpr = returnStmt->getRetValue();
+                if (const clang::ExprWithCleanups *exprWithCleanups = dyn_cast<ExprWithCleanups>(returnExpr)) {
+                    if (auto opCallExpr = llvm::dyn_cast<clang::CXXOperatorCallExpr>(exprWithCleanups->getSubExpr())) {
+                        const clang::Expr *lhs = opCallExpr->getArg(0);
+                        if (auto *materializeExpr = llvm::dyn_cast<MaterializeTemporaryExpr>(lhs)) {
+                            if (auto *implicitCastExpr = llvm::dyn_cast<ImplicitCastExpr>(materializeExpr->getSubExpr())) {
+                                if (auto *lhsCall = dyn_cast<CallExpr>(implicitCastExpr->getSubExpr())) {
+                                    const auto &lhsArgs = lhsCall->arguments();
+
+                                    size_t numLhsArgs = std::distance(lhsArgs.begin(), lhsArgs.end());
+                                    if (classFields.size() != numLhsArgs) {
+                                        m_check->emitWarning(callExpr->getExprLoc(), "Comparison operator does not use all member variables");
+                                    }
+                                }
+                            }
+                        }
+                    }
+                }
+
+                if (const BinaryOperator *binOp = dyn_cast<BinaryOperator>(returnExpr)) {
+                    checkBinaryOperator(binOp, classDecl, classFields);
+                    if (!classFields.empty()) {
+                        m_check->emitWarning(callExpr->getExprLoc(), "Comparison operator does not use all member variables");
+                    }
+                }
+            }
+        }
+    }
+};
+
+CompareMemberCheck::CompareMemberCheck(const std::string &name, Options options)
+    : CheckBase(name, options)
+    , m_astMatcherCallBack(std::make_unique<Caller>(this))
+{
+}
+
+CompareMemberCheck::~CompareMemberCheck() = default;
+
+void CompareMemberCheck::VisitStmt(Stmt *stmt)
+{
+    auto call = dyn_cast<CXXOperatorCallExpr>(stmt);
+    if (!call || call->getNumArgs() != 1)
+        return;
+}
+
+void CompareMemberCheck::registerASTMatchers(MatchFinder &finder)
+{
+    finder.addMatcher(cxxOperatorCallExpr().bind("callExpr"), m_astMatcherCallBack.get());
+}
diff --git a/src/checks/manuallevel/compare-member-check.h b/src/checks/manuallevel/compare-member-check.h
new file mode 100644
index 00000000..12fd7cf6
--- /dev/null
+++ b/src/checks/manuallevel/compare-member-check.h
@@ -0,0 +1,57 @@
+/*
+    This file is part of the clazy static checker.
+
+    Copyright (C) 2023 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com
+    Author: Shivam Kunwar <shivam.kunwar@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_COMPARE_MEMBER_CHECK_H
+#define CLAZY_COMPARE_MEMBER_CHECK_H
+
+#include "checkbase.h"
+
+#include <string>
+
+class Caller;
+class ClazyContext;
+
+namespace clang
+{
+namespace ast_matchers
+{
+class MatchFinder;
+} // namespace ast_matchers
+class Stmt;
+class VarDecl;
+class CXXRecordDecl;
+class QualType;
+} // namespace clang
+
+class CompareMemberCheck : public CheckBase
+{
+public:
+    explicit CompareMemberCheck(const std::string &name, Options options);
+    ~CompareMemberCheck();
+    void VisitStmt(clang::Stmt *stmt) override;
+    void registerASTMatchers(clang::ast_matchers::MatchFinder &) override;
+
+private:
+    std::unique_ptr<ClazyAstMatcherCallback> m_astMatcherCallBack; // TODO: add std::propagate_const
+};
+
+#endif
diff --git a/tests/compare-member-check/config.json b/tests/compare-member-check/config.json
new file mode 100644
index 00000000..21a8a1f7
--- /dev/null
+++ b/tests/compare-member-check/config.json
@@ -0,0 +1,7 @@
+{
+    "tests" : [
+        {
+            "filename" : "main.cpp"
+        }
+    ]
+}
+\ No newline at end of file
diff --git a/tests/compare-member-check/main.cpp b/tests/compare-member-check/main.cpp
new file mode 100644
index 00000000..143f473f
--- /dev/null
+++ b/tests/compare-member-check/main.cpp
@@ -0,0 +1,54 @@
+#include <tuple>
+
+class ExampleClass
+{
+public:
+    int a;
+    int b;
+    int c;
+
+    bool operator==(const ExampleClass &other) const
+    {
+        return a == other.a || b == other.b || c == other.c; // No Warning, Using all the member variables in comparison
+    }
+
+    bool operator>(const ExampleClass &other) const
+    {
+        return a > other.a || b > other.b; // Warning, Using only two member variables in the comparison
+    }
+
+    bool operator<(const ExampleClass &rhs) const
+    {
+        return std::tie(a, b, c) < std::tie(rhs.a, rhs.b, rhs.c); // No Warning, Using all the member variables in comparison
+    }
+    bool operator>=(const ExampleClass &rhs) const
+    {
+        return std::tie(a) >= std::tie(rhs.a); // Warning, Using only one member variable in comparison
+    }
+};
+
+int main()
+{
+    ExampleClass object1;
+    ExampleClass object2;
+
+    object1.a = 5;
+    object1.b = 10;
+    object1.c = 15;
+
+    object2.a = 5;
+    object2.b = 20;
+    object2.c = 30;
+
+    if (object1 == object2) { // Should not trigger a warning from the checker
+    }
+
+    if (object1 > object2) { // Should trigger a warning from the checker
+    }
+    if (object1 < object2) { // Should not trigger a warning from the checker
+    }
+    if (object1 >= object2) { // Should trigger a warning from the checker
+    }
+
+    return 0;
+}
+\ No newline at end of file
diff --git a/tests/compare-member-check/main.cpp.expected b/tests/compare-member-check/main.cpp.expected
new file mode 100644
index 00000000..cafb258f
--- /dev/null
+++ b/tests/compare-member-check/main.cpp.expected
@@ -0,0 +1,2 @@
+compare-member-check/main.cpp:46:17: warning: Comparison operator does not use all member variables [-Wclazy-compare-member-check]
+compare-member-check/main.cpp:50:17: warning: Comparison operator does not use all member variables [-Wclazy-compare-member-check]
author	Shivam kunwar <shivam.kunwar@kdab.com>	2023-02-12 20:38:37 +0530
committer	Alexander Lohnau <alexander.lohnau@gmx.de>	2025-09-22 21:43:36 +0200
commit	123116bba09ba001ef752195c0f780f0bfc7953e (patch)
tree	877121b67485a8c36aafa3f5c4f4aec17cfad37f
parent	9b3707939322f8ae25b7fdc2f13a453eed9d71ee (diff)