https://github.com/balazske updated
https://github.com/llvm/llvm-project/pull/132242
From e3064b600ea726ab7b3dea054e9f11e1ce028297 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Wed, 19 Mar 2025 16:09:04 +0100
Subject: [PATCH 1/2] [clang-tidy] Add check
bugprone-misleading-setter-of-reference
---
.../bugprone/BugproneTidyModule.cpp | 3 +
.../clang-tidy/bugprone/CMakeLists.txt | 1 +
.../MisleadingSetterOfReferenceCheck.cpp | 58 +++++++++++++++++++
.../MisleadingSetterOfReferenceCheck.h | 37 ++++++++++++
.../misleading-setter-of-reference.rst | 42 ++++++++++++++
.../misleading-setter-of-reference.cpp | 50 ++++++++++++++++
6 files changed, 191 insertions(+)
create mode 100644
clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
create mode 100644
clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h
create mode 100644
clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index b780a85bdf3fe..64f4a524daf0d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -41,6 +41,7 @@
#include "LambdaFunctionNameCheck.h"
#include "MacroParenthesesCheck.h"
#include "MacroRepeatedSideEffectsCheck.h"
+#include "MisleadingSetterOfReferenceCheck.h"
#include "MisplacedOperatorInStrlenInAllocCheck.h"
#include "MisplacedPointerArithmeticInAllocCheck.h"
#include "MisplacedWideningCastCheck.h"
@@ -170,6 +171,8 @@ class BugproneModule : public ClangTidyModule {
"bugprone-macro-parentheses");
CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
"bugprone-macro-repeated-side-effects");
+ CheckFactories.registerCheck<MisleadingSetterOfReferenceCheck>(
+ "bugprone-misleading-setter-of-reference");
CheckFactories.registerCheck<MisplacedOperatorInStrlenInAllocCheck>(
"bugprone-misplaced-operator-in-strlen-in-alloc");
CheckFactories.registerCheck<MisplacedPointerArithmeticInAllocCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index e310ea9c94543..d862794cde323 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -42,6 +42,7 @@ add_clang_library(clangTidyBugproneModule STATIC
LambdaFunctionNameCheck.cpp
MacroParenthesesCheck.cpp
MacroRepeatedSideEffectsCheck.cpp
+ MisleadingSetterOfReferenceCheck.cpp
MisplacedOperatorInStrlenInAllocCheck.cpp
MisplacedPointerArithmeticInAllocCheck.cpp
MisplacedWideningCastCheck.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
new file mode 100644
index 0000000000000..043d15e7fead2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
@@ -0,0 +1,58 @@
+//===--- MisleadingSetterOfReferenceCheck.cpp -
clang-tidy-----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "MisleadingSetterOfReferenceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void MisleadingSetterOfReferenceCheck::registerMatchers(MatchFinder *Finder) {
+ auto RefField =
+ fieldDecl(unless(isPublic()),
+ hasType(referenceType(pointee(equalsBoundNode("type")))))
+ .bind("member");
+ auto AssignLHS =
+ memberExpr(hasObjectExpression(cxxThisExpr()), member(RefField));
+ auto DerefOperand = expr(ignoringParenImpCasts(
+ declRefExpr(to(parmVarDecl(equalsBoundNode("parm"))))));
+ auto AssignRHS = expr(ignoringParenImpCasts(
+ unaryOperator(hasOperatorName("*"), hasUnaryOperand(DerefOperand))));
+
+ auto BinaryOpAssign = binaryOperator(hasOperatorName("="), hasLHS(AssignLHS),
+ hasRHS(AssignRHS));
+ auto CXXOperatorCallAssign = cxxOperatorCallExpr(
+ hasOverloadedOperatorName("="), hasLHS(AssignLHS), hasRHS(AssignRHS));
+
+ auto SetBody =
+ compoundStmt(statementCountIs(1),
+ anyOf(has(BinaryOpAssign), has(CXXOperatorCallAssign)));
+ auto BadSetFunction =
+ cxxMethodDecl(parameterCountIs(1), isPublic(),
+ hasAnyParameter(parmVarDecl(hasType(pointerType(pointee(
+ qualType().bind("type")))))
+ .bind("parm")),
+ hasBody(SetBody))
+ .bind("bad-set-function");
+ Finder->addMatcher(BadSetFunction, this);
+}
+
+void MisleadingSetterOfReferenceCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *Found =
Result.Nodes.getNodeAs<CXXMethodDecl>("bad-set-function");
+ const auto *Member = Result.Nodes.getNodeAs<FieldDecl>("member");
+
+ diag(Found->getBeginLoc(),
+ "function \'%0\' can be mistakenly used in order to change the "
+ "reference \'%1\' instead of the value of it")
+ << Found->getName() << Member->getName();
+}
+
+} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h
b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h
new file mode 100644
index 0000000000000..99e7a9435cfa9
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.h
@@ -0,0 +1,37 @@
+//===--- MisleadingSetterOfReferenceCheck.h - clang-tidy---------*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISLEADINGSETTEROFREFERENCECHECK_H
+#define
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISLEADINGSETTEROFREFERENCECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Emits a warning when a setter-like function that has a pointer parameter
+/// is used to set value of a field with reference type.
+///
+/// For the user-facing documentation see:
+///
http://clang.llvm.org/extra/clang-tidy/checks/bugprone/throw-keyword-missing.html
+class MisleadingSetterOfReferenceCheck : public ClangTidyCheck {
+public:
+ MisleadingSetterOfReferenceCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ std::optional<TraversalKind> getCheckTraversalKind() const override {
+ return TK_IgnoreUnlessSpelledInSource;
+ }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif //
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISLEADINGSETTEROFREFERENCECHECK_H
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
new file mode 100644
index 0000000000000..69c0d5530fb61
--- /dev/null
+++
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
@@ -0,0 +1,42 @@
+.. title:: clang-tidy - bugprone-misleading-setter-of-reference
+
+bugprone-misleading-setter-of-reference
+=======================================
+
+Finds setter-like functions that take a pointer parameter and set a (non-const)
+reference with the pointed value. The fact that a setter function takes a
+pointer might cause the belief that an internal reference (if it would be a
+pointer) is changed instead of the pointed-to (or referenced) value.
+
+Only member functions are detected which have a single parameter and contain a
+single (maybe overloaded) assignment operator call.
+
+Example:
+
+.. code-block:: c++
+
+ class Widget {
+ int& ref_; // non-const reference member
+ public:
+ // non-copyable
+ Widget(const Widget&) = delete;
+ // non-movable
+ Widget(Widget&&) = delete;
+
+ Widget(int& value) : ref_(value) {}
+
+ // Potentially dangerous setter that could lead to unintended behaviour
+ void setRef(int* ptr) {
+ ref_ = *ptr; // This assigns to the referenced value, not changing
what ref_ references
+ }
+ };
+
+ int main() {
+ int value1 = 42;
+ int value2 = 100;
+ Widget w(value1);
+
+ // This might look like it changes what ref_ references to,
+ // but it actually modifies value1 to be 100
+ w.setRef(&value2); // value1 is now 100, ref_ still references value1
+ }
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
new file mode 100644
index 0000000000000..9f9a616824469
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s bugprone-misleading-setter-of-reference %t
+
+struct X {
+ X &operator=(const X &) { return *this; }
+ int &Mem;
+};
+
+class Test1 {
+ X &MemX;
+ int &MemI;
+protected:
+ long &MemL;
+public:
+ long &MemLPub;
+
+ Test1(X &MemX, int &MemI, long &MemL) : MemX(MemX), MemI(MemI), MemL(MemL),
MemLPub(MemL) {}
+ void setI(int *NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setI' can be
mistakenly used in order to change the reference 'MemI' instead of the value of
it [bugprone-misleading-setter-of-reference]
+ MemI = *NewValue;
+ }
+ void setL(long *NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setL' can be
mistakenly used in order to change the reference 'MemL' instead of the value of
it [bugprone-misleading-setter-of-reference]
+ MemL = *NewValue;
+ }
+ void setX(X *NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setX' can be
mistakenly used in order to change the reference 'MemX' instead of the value of
it [bugprone-misleading-setter-of-reference]
+ MemX = *NewValue;
+ }
+ void set1(int *NewValue) {
+ MemX.Mem = *NewValue;
+ }
+ void set2(int *NewValue) {
+ MemL = static_cast<long>(*NewValue);
+ }
+ void set3(int *NewValue) {
+ MemI = *NewValue;
+ MemL = static_cast<long>(*NewValue);
+ }
+ void set4(long *NewValue, int) {
+ MemL = *NewValue;
+ }
+ void setLPub(long *NewValue) {
+ MemLPub = *NewValue;
+ }
+
+protected:
+ void set5(long *NewValue) {
+ MemL = *NewValue;
+ }
+};
From 2a7b326318f4dcab3647337a398977f69a1fe662 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Thu, 20 Mar 2025 17:27:55 +0100
Subject: [PATCH 2/2] updates from review and improvements
---
.../MisleadingSetterOfReferenceCheck.cpp | 30 ++++----
.../misleading-setter-of-reference.rst | 33 +++++----
.../misleading-setter-of-reference.cpp | 71 ++++++++++++++++++-
3 files changed, 104 insertions(+), 30 deletions(-)
diff --git
a/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
index 043d15e7fead2..212c24440c594 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
@@ -16,14 +16,14 @@ namespace clang::tidy::bugprone {
void MisleadingSetterOfReferenceCheck::registerMatchers(MatchFinder *Finder) {
auto RefField =
- fieldDecl(unless(isPublic()),
- hasType(referenceType(pointee(equalsBoundNode("type")))))
+ fieldDecl(unless(isPublic()), hasType(hasCanonicalType(referenceType(
+ pointee(equalsBoundNode("type"))))))
.bind("member");
- auto AssignLHS =
- memberExpr(hasObjectExpression(cxxThisExpr()), member(RefField));
- auto DerefOperand = expr(ignoringParenImpCasts(
+ auto AssignLHS = memberExpr(
+ hasObjectExpression(ignoringParenCasts(cxxThisExpr())),
member(RefField));
+ auto DerefOperand = expr(ignoringParenCasts(
declRefExpr(to(parmVarDecl(equalsBoundNode("parm"))))));
- auto AssignRHS = expr(ignoringParenImpCasts(
+ auto AssignRHS = expr(ignoringParenCasts(
unaryOperator(hasOperatorName("*"), hasUnaryOperand(DerefOperand))));
auto BinaryOpAssign = binaryOperator(hasOperatorName("="), hasLHS(AssignLHS),
@@ -35,11 +35,14 @@ void
MisleadingSetterOfReferenceCheck::registerMatchers(MatchFinder *Finder) {
compoundStmt(statementCountIs(1),
anyOf(has(BinaryOpAssign), has(CXXOperatorCallAssign)));
auto BadSetFunction =
- cxxMethodDecl(parameterCountIs(1), isPublic(),
- hasAnyParameter(parmVarDecl(hasType(pointerType(pointee(
- qualType().bind("type")))))
- .bind("parm")),
- hasBody(SetBody))
+ cxxMethodDecl(
+ parameterCountIs(1), isPublic(),
+ hasParameter(
+ 0,
+
parmVarDecl(hasType(hasCanonicalType(pointerType(pointee(qualType(
+ hasCanonicalType(qualType().bind("type"))))))))
+ .bind("parm")),
+ hasBody(SetBody))
.bind("bad-set-function");
Finder->addMatcher(BadSetFunction, this);
}
@@ -50,8 +53,9 @@ void MisleadingSetterOfReferenceCheck::check(
const auto *Member = Result.Nodes.getNodeAs<FieldDecl>("member");
diag(Found->getBeginLoc(),
- "function \'%0\' can be mistakenly used in order to change the "
- "reference \'%1\' instead of the value of it")
+ "function '%0' can be mistakenly used in order to change the "
+ "reference '%1' instead of the value of it; consider not using a "
+ "pointer as argument")
<< Found->getName() << Member->getName();
}
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
index 69c0d5530fb61..90537cf64f4cf 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
@@ -3,13 +3,17 @@
bugprone-misleading-setter-of-reference
=======================================
-Finds setter-like functions that take a pointer parameter and set a (non-const)
-reference with the pointed value. The fact that a setter function takes a
-pointer might cause the belief that an internal reference (if it would be a
-pointer) is changed instead of the pointed-to (or referenced) value.
+Finds setter-like member functions that take a pointer parameter and set a
+(non-const) reference member of the same class with the pointed value.
+
+The factthat a setter function takes a pointer might cause the belief that an
+internal reference (if it would be a pointer) is changed instead of the
+pointed-to (or referenced) value.
Only member functions are detected which have a single parameter and contain a
-single (maybe overloaded) assignment operator call.
+single (maybe overloaded) assignment operator call. The changed member variable
+must be private (or protected) for the checker to detect the fault (a public
+member can be changed anyway without a set function).
Example:
@@ -18,15 +22,10 @@ Example:
class Widget {
int& ref_; // non-const reference member
public:
- // non-copyable
- Widget(const Widget&) = delete;
- // non-movable
- Widget(Widget&&) = delete;
-
- Widget(int& value) : ref_(value) {}
-
- // Potentially dangerous setter that could lead to unintended behaviour
- void setRef(int* ptr) {
+ Widget(int &value) : ref_(value) {}
+
+ // Warning: Potentially dangerous setter that could lead to unintended
behaviour
+ void setRef(int *ptr) {
ref_ = *ptr; // This assigns to the referenced value, not changing
what ref_ references
}
};
@@ -40,3 +39,9 @@ Example:
// but it actually modifies value1 to be 100
w.setRef(&value2); // value1 is now 100, ref_ still references value1
}
+
+Possible fixes:
+ - Change the parameter of the "set" function to non-pointer or const
reference
+ type.
+ - Change the type of the member variable to a pointer and in the "set"
+ function assign a value to the pointer (without dereference).
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
index 9f9a616824469..59d67f1d68561 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/misleading-setter-of-reference.cpp
@@ -2,7 +2,9 @@
struct X {
X &operator=(const X &) { return *this; }
+private:
int &Mem;
+ friend class Test1;
};
class Test1 {
@@ -15,15 +17,15 @@ class Test1 {
Test1(X &MemX, int &MemI, long &MemL) : MemX(MemX), MemI(MemI), MemL(MemL),
MemLPub(MemL) {}
void setI(int *NewValue) {
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setI' can be
mistakenly used in order to change the reference 'MemI' instead of the value of
it [bugprone-misleading-setter-of-reference]
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setI' can be
mistakenly used in order to change the reference 'MemI' instead of the value of
it
MemI = *NewValue;
}
void setL(long *NewValue) {
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setL' can be
mistakenly used in order to change the reference 'MemL' instead of the value of
it [bugprone-misleading-setter-of-reference]
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setL' can be
mistakenly used in order to change the reference 'MemL' instead of the value of
it
MemL = *NewValue;
}
void setX(X *NewValue) {
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setX' can be
mistakenly used in order to change the reference 'MemX' instead of the value of
it [bugprone-misleading-setter-of-reference]
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setX' can be
mistakenly used in order to change the reference 'MemX' instead of the value of
it
MemX = *NewValue;
}
void set1(int *NewValue) {
@@ -48,3 +50,66 @@ class Test1 {
MemL = *NewValue;
}
};
+
+class Base {
+protected:
+ int &MemI;
+};
+
+class Derived : public Base {
+public:
+ void setI(int *NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setI' can be
mistakenly used in order to change the reference 'MemI' instead of the value of
it
+ MemI = *NewValue;
+ }
+};
+
+using UIntRef = unsigned int &;
+using UIntPtr = unsigned int *;
+using UInt = unsigned int;
+
+class AliasTest {
+ UIntRef Value1;
+ UInt &Value2;
+ unsigned int &Value3;
+public:
+ void setValue1(UIntPtr NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue1' can be
mistakenly used in order to change the reference 'Value1' instead of the value
of it
+ Value1 = *NewValue;
+ }
+ void setValue2(unsigned int *NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue2' can be
mistakenly used in order to change the reference 'Value2' instead of the value
of it
+ Value2 = *NewValue;
+ }
+ void setValue3(UInt *NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue3' can be
mistakenly used in order to change the reference 'Value3' instead of the value
of it
+ Value3 = *NewValue;
+ }
+};
+
+template <typename T>
+class TemplateTest {
+ T &Mem;
+public:
+ void setValue(T *NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue' can be
mistakenly used in order to change the reference 'Mem' instead of the value of
it
+ Mem = *NewValue;
+ }
+};
+
+void f_TTChar(TemplateTest<char> *);
+void f_TTChar(TemplateTest<float> *);
+
+template <typename T>
+class AddMember {
+protected:
+ T &Value;
+};
+
+class TemplateBaseTest : public AddMember<int> {
+public:
+ void setValue(int *NewValue) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'setValue' can be
mistakenly used in order to change the reference 'Value' instead of the value
of it
+ Value = *NewValue;
+ }
+};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
