[clang-tidy] Add check bugprone-misleading-setter-of-reference #132242
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra Author: Balázs Kéri (balazske) ChangesThere can be concerns about the usefulness of this check but for some code it can be useful. Full diff: https://github.com/llvm/llvm-project/pull/132242.diff 6 Files Affected:
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;
+ }
+};
|
|
@llvm/pr-subscribers-clang-tidy Author: Balázs Kéri (balazske) ChangesThere can be concerns about the usefulness of this check but for some code it can be useful. Full diff: https://github.com/llvm/llvm-project/pull/132242.diff 6 Files Affected:
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;
+ }
+};
|
|
Please add entry in Release Notes. |
NagyDonat
reviewed
Mar 25, 2025
clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
Outdated
Show resolved
Hide resolved
vbvictor
reviewed
Apr 1, 2025
| @@ -0,0 +1,50 @@ | ||
| // RUN: %check_clang_tidy %s bugprone-misleading-setter-of-reference %t |
There was a problem hiding this comment.
Consider adding tests for inheritance. E.g. Base class has method SetX and Derived calls it.
There was a problem hiding this comment.
Also, a test with templated class and templated method could be useful.
There was a problem hiding this comment.
Added tests for inheritance and templates.
| const auto *Member = Result.Nodes.getNodeAs<FieldDecl>("member"); | ||
|
|
||
| diag(Found->getBeginLoc(), | ||
| "function \'%0\' can be mistakenly used in order to change the " |
There was a problem hiding this comment.
We should also specify how a user can make code correct and avoid this warning.
e.g. "function setX can be mistakenly used in order to change the reference MemX instead of the value of it; consider changing type of parameter NewValue to X".
AFAIK this is desired behavior of setX.
clang-tools-extra/clang-tidy/bugprone/MisleadingSetterOfReferenceCheck.cpp
Outdated
Show resolved
Hide resolved
| compoundStmt(statementCountIs(1), | ||
| anyOf(has(BinaryOpAssign), has(CXXOperatorCallAssign))); | ||
| auto BadSetFunction = | ||
| cxxMethodDecl(parameterCountIs(1), isPublic(), |
There was a problem hiding this comment.
isPublic may not be necessary here imo
There was a problem hiding this comment.
The intent is to find cases when the reference is changed from outside the class with a public function.
There was a problem hiding this comment.
I support keeping isPublic because this check essentially looks for classes with a misleading API, and these functions are bugprone because they can mislead somebody who only looks at the public signature without reviewing the actual implementation -- and these don't apply for private methods.
There was a problem hiding this comment.
Consider this example https://godbolt.org/z/9xd4j9ef5, I can call a protected method as a public, maybe add isProteced() as well?
There was a problem hiding this comment.
I think exposing a protected setter method (which has this particular bug) as a public method of a derived class would be very rare, but I'm not opposed to adding isProtected() if you think that it would be more elegant.
I can also accept removing visibility constraints on the method -- private setter methods should be vanishingly rare in real code, so although excluding them makes sense from a theoretical/design POV, it doesn't provide practical benefits...
There was a problem hiding this comment.
I'm more in favor of removing any visibility.
Even if public API is OK, maybe the class author made a mistake in internal realization and now there is actually a bug. I think we should check for that as well.
There was a problem hiding this comment.
I removed it now, this function design is "suspicious" even if protected or private because it is always better to pass the parameter by value or reference.
clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
Outdated
Show resolved
Hide resolved
|
The commit looks promising, my only remaining remarks are about phrasing in the documentation and comments. |
NagyDonat
reviewed
May 6, 2025
clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
Outdated
Show resolved
Hide resolved
| compoundStmt(statementCountIs(1), | ||
| anyOf(has(BinaryOpAssign), has(CXXOperatorCallAssign))); | ||
| auto BadSetFunction = | ||
| cxxMethodDecl(parameterCountIs(1), isPublic(), |
There was a problem hiding this comment.
I support keeping isPublic because this check essentially looks for classes with a misleading API, and these functions are bugprone because they can mislead somebody who only looks at the public signature without reviewing the actual implementation -- and these don't apply for private methods.
clang-tools-extra/docs/clang-tidy/checks/bugprone/misleading-setter-of-reference.rst
Outdated
Show resolved
Hide resolved
|
Please add entry to |
| 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 checker detects public member functions that have a single parameter (which |
There was a problem hiding this comment.
Suggested change
| The checker detects public member functions that have a single parameter (which | |
| The check detects public member functions that have a single parameter (which |
Clang-Tidy uses check.
NagyDonat
approved these changes
May 7, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/16760 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There can be concerns about the usefulness of this check but for some code it can be useful.