updates from review and improvements

Author: balazske

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();
 }
 

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).

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;
+  }
+};