[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360540: [clang-tidy] new check: 
bugprone-unhandled-self-assignment (authored by ztamas, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60507?vs=198789=199166#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -46,6 +46,7 @@
 #include "TooSmallLoopVariableCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
+#include "UnhandledSelfAssignmentCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
@@ -132,6 +133,8 @@
 "bugprone-undefined-memory-manipulation");
 CheckFactories.registerCheck(
 "bugprone-undelegated-constructor");
+CheckFactories.registerCheck(
+"bugprone-unhandled-self-assignment");
 CheckFactories.registerCheck(
 "bugprone-unused-raii");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
@@ -38,6 +38,7 @@
   TooSmallLoopVariableCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
+  UnhandledSelfAssignmentCheck.cpp
   UnusedRaiiCheck.cpp
   UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -0,0 +1,99 @@
+//===--- UnhandledSelfAssignmentCheck.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 "UnhandledSelfAssignmentCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+return;
+
+  // We don't care about deleted, default or implicit operator implementations.
+  const auto IsUserDefined = cxxMethodDecl(
+  isDefinition(), unless(anyOf(isDeleted(), isImplicit(), isDefaulted(;
+
+  // We don't need to worry when a copy assignment operator gets the other
+  // object by value.
+  const auto HasReferenceParam =
+  cxxMethodDecl(hasParameter(0, parmVarDecl(hasType(referenceType();
+
+  // Self-check: Code compares something with 'this' pointer. We don't check
+  // whether it is actually the parameter what we compare.
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant(
+  binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ has(ignoringParenCasts(cxxThisExpr()));
+
+  // Both copy-and-swap and copy-and-move method creates a copy first and
+  // assign it to 'this' with swap or move.
+  // In the non-template case, we can search for the copy constructor call.
+  const auto HasNonTemplateSelfCopy = cxxMethodDecl(
+  ofClass(cxxRecordDecl(unless(hasAncestor(classTemplateDecl(),
+  hasDescendant(cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
+  isCopyConstructor(), ofClass(equalsBoundNode("class")));
+
+  // In the template case, we need to handle two separate cases: 1) a local
+  // variable is created with the copy, 2) copy is created only as a temporary
+  // object.
+  const auto HasTemplateSelfCopy = cxxMethodDecl(
+  

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas added a comment.

> I definitely want to see the diagnostic text changed away from "might be" -- 
> that's too noncommittal for my tastes. I'd also like to see the additional 
> test case (I suspect it will just work out of the box, but if it doesn't, it 
> would be good to know about it). The CERT alias is a bit more of a grey area 
> -- I'd like to insist on it being added because it's trivial, it improves the 
> behavior of this check by introducing an option some users will want, and 
> checks conforming to coding standards are always more compelling than one-off 
> checks. However, you seem very resistant to that and I don't want to add to 
> your work load if you are opposed to doing it, so I don't insist on the 
> change.

During LibreOffice development, I'm socialized to keep a patch as small as 
possible. Versioning history can be more "clean" with smaller patches. For 
example, it's easier to find and fix regressions
with bisecting. Also smaller patches make the review process faster, etc. 
That's why I'm trying to separate what is absolutely necessary to include and 
what can be cut off from this patch.

I changed the warning message and I added the requested test case. This test 
case is ignored by the check as expected.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 198789.
ztamas added a comment.

copy operator -> copy assignment operator


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,579 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+class NotSelfCheck {
+public:
+  NotSelfCheck =(const NotSelfCheck ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+if ( == this->doSomething()) {
+  // ...
+}
+return *this;
+  }
+
+  void *doSomething() {
+return p;
+  }
+
+private:
+  int *p;
+};
+
+template 
+class TemplatePtrField {
+public:
+  TemplatePtrField =(const TemplatePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:24: warning: 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 198787.
ztamas added a comment.

Changed "might not" to "does not" in the warning message.
Added the requested test case with the swapped template arguments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,579 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+class NotSelfCheck {
+public:
+  NotSelfCheck =(const NotSelfCheck ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+if ( == this->doSomething()) {
+  // ...
+}
+return *this;
+  }
+
+  void *doSomething() {
+return p;
+  }
+
+private:
+  int *p;
+};
+
+template 
+class TemplatePtrField {
+public:
+  

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D60507#1491095 , @ztamas wrote:

> Ping.
>  Is it good to go or is there anything else I need to include in this patch?


Sorry for the delay, I was gone for WG14 meetings last week, which made 
reviewing more involved patches somewhat difficult. I'm back now and starting 
to dig out from under the mountain of emails.

> Among the lots of idea, I'm not sure which is just brainstorming and which is 
> a change request.

Generally speaking, all feedback is a bit of a mixture of the two. They're 
change requests, but we can definitely brainstorm whether the request makes 
sense, how to do it, whether there are better changes to make instead, etc. We 
usually say something explicit like "but I don't insist" when we're just 
spitballing ideas. That said, I'm sorry if I was unclear on what changes I was 
insisting on.

I definitely want to see the diagnostic text changed away from "might be" -- 
that's too noncommittal for my tastes. I'd also like to see the additional test 
case (I suspect it will just work out of the box, but if it doesn't, it would 
be good to know about it). The CERT alias is a bit more of a grey area -- I'd 
like to insist on it being added because it's trivial, it improves the behavior 
of this check by introducing an option some users will want, and checks 
conforming to coding standards are always more compelling than one-off checks. 
However, you seem very resistant to that and I don't want to add to your work 
load if you are opposed to doing it, so I don't insist on the change.

> The check seems to work (has useful catches and does not produce too many 
> false positives), however, it does not conform with the corresponding cert 
> rule. I expect that a cert alias with an option can be added in a follow-up 
> patch (option to ignore ThisHasSuspiciousField pattern). The bugprone-* check 
> would have the same default behavior as it has now in this patch. So adding 
> the cert alias would not change this behavior, right? In this case, this code 
> change can be added in a separate patch I guess.

Agreed.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+  diag(MatchedDecl->getLocation(),
+   "operator=() might not handle self-assignment properly");
+}

ztamas wrote:
> aaron.ballman wrote:
> > ztamas wrote:
> > > aaron.ballman wrote:
> > > > Hmm, the "might not" seems a bit flat to me. How about: `'operator=()' 
> > > > does not properly test for self-assignment`?
> > > > 
> > > > Also, do we want to have a fix-it to insert a check for self assignment 
> > > > at the start of the function?
> > > I don't think "test for self-assignment" will be good, because it's only 
> > > one way to make the operator self assignment safe. In case of 
> > > copy-and-swap and copy-and-move there is no testing of self assignment, 
> > > but swaping/moving works in all cases without explicit self check.
> > > 
> > > A fix-it can be a good idea for a follow-up patch.
> > Good point on the use of "test" in the diagnostic. My concern is more with 
> > it sounding like we don't actually know -- the "might" is what's bothering 
> > me. I think if we switch it to "does not", it'd be a bit better.
> We don't actually know. We check only some patterns, but there might be other 
> operator=() implementations which are self assignment safe (see false 
> positive test cases).
I understand that we're using a heuristic, but that doesn't mean we use 
language like "might not" in the resulting diagnostic. Either we think the code 
is correct (we don't diagnose) or the code doesn't do what it needs to do to 
pass our check (we diagnose). That's why I recommended "does not" -- the 
operator=() does not handle self-assignment properly according to your check. I 
couldn't find any existing diagnostics saying the code "might not" do something 
properly.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:214
+};
+
+///

Does the check diagnose code like this?
```
template 
class C {
  Ty *Ptr1;
  Uy *Ptr2;

public:
  C& operator=(const C ) {
Ptr1 = RHS.getUy();
Ptr2 = RHS.getTy();
return *this;
  }

  Ty *getTy() const { return Ptr1; }
  Uy *getUy() const { return Ptr2; }
};
```
I'm hoping we don't consider it a copy assignment operator and thus don't 
diagnose (note that the template argument order is reversed in the assignment 
operator parameter type).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas added a comment.

Ping.
Is it good to go or is there anything else I need to include in this patch?
Among the lots of idea, I'm not sure which is just brainstorming and which is a 
change request.
The check seems to work (has useful catches and does not produce too many false 
positives), however, it does not conform with the corresponding cert rule. I 
expect that a cert alias with an option can be added in a follow-up patch 
(option to ignore ThisHasSuspiciousField pattern). The bugprone-* check would 
have the same default behavior as it has now in this patch. So adding the cert 
alias would not change this behavior, right? In this case, this code change can 
be added in a separate patch I guess.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 197014.
ztamas added a comment.

Better handling of template and non-template self-copy


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,561 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+class NotSelfCheck {
+public:
+  NotSelfCheck =(const NotSelfCheck ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+if ( == this->doSomething()) {
+  // ...
+}
+return *this;
+  }
+
+  void *doSomething() {
+return p;
+  }
+
+private:
+  int *p;
+};
+
+template 
+class TemplatePtrField {
+public:
+  TemplatePtrField =(const TemplatePtrField ) {
+// CHECK-MESSAGES: 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas added a comment.

Ok, is there anything else should be included in this patch?
I saw more ideas about how to extend the functionality (custom pointers, 
fix-it, etc). I interpreted these ideas as nice-to-have things for the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 196410.
ztamas added a comment.

Remove outdated comment from docs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,521 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+class NotSelfCheck {
+public:
+  NotSelfCheck =(const NotSelfCheck ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+if ( == this->doSomething()) {
+  // ...
+}
+return *this;
+  }
+
+  void *doSomething() {
+return p;
+  }
+
+private:
+  int *p;
+};
+
+template 
+class TemplatePtrField {
+public:
+  TemplatePtrField =(const TemplatePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:24: 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 8 inline comments as done.
ztamas added a comment.

Mark some comments Done, which were handled some way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas added a comment.

I also reformatted the code with clang-format.

So now the templates are handled, however, it's still not fit with the cert 
rule because we not catch all classes, but only those who have suspicious 
fields. I think this one can be added in a follow-up patch and then this check 
can be added to the cert module as an alias.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 196295.
ztamas added a comment.

Make the check to work with templates too


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,521 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+class NotSelfCheck {
+public:
+  NotSelfCheck =(const NotSelfCheck ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+if ( == this->doSomething()) {
+  // ...
+}
+return *this;
+  }
+
+  void *doSomething() {
+return p;
+  }
+
+private:
+  int *p;
+};
+
+template 
+class TemplatePtrField {
+public:
+  TemplatePtrField =(const TemplatePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:24: 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 3 inline comments as done.
ztamas added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:62-64
+  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+  has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
+  hasType(arrayType(;

aaron.ballman wrote:
> ztamas wrote:
> > aaron.ballman wrote:
> > > Hmm, while I understand why you're doing this, I'm worried that it will 
> > > miss some pretty important cases. For instance, improper thread locking 
> > > could result in deadlocks, improper releasing of non-memory resources 
> > > could be problematic (such as network connections, file streams, etc), 
> > > even simple integer assignments could be problematic in theory:
> > > ```
> > > Yobble& Yobble::operator=(const Yobble ) {
> > >   superSecretHashVal = 0; // Being secure!
> > >   ... some code that may early return ...
> > >   superSecretHashVal = Y.superSecretHashVal;
> > > }
> > > ```
> > > I wonder whether we want an option here to allow users to diagnose 
> > > regardless of whether we think it's suspicious or not.
> > > 
> > > At the very least, this code should not be enabled for the CERT version 
> > > of the check as it doesn't conform to the CERT requirements.
> > It's starting to be too much for me.
> > First, the problem was false positives. If there are too many false 
> > positives then better to have it in the cert module.
> > Now your problem is that this is not fit with the CERT requirements, 
> > because of this matcher which reduces the false positives. Adding this 
> > check to the CERT module was not my idea in the first place. So I suggest 
> > to have it a simple bugprone check  (and remove the cert alias) and also we 
> > can mention that it is related to the corresponding cert rule (but it does 
> > not conform with it entirely).
> > This check allows the usage of copy-and-move pattern, which does not 
> > conform with the cert specification either where only the self-check and 
> > copy-and-swap is mentioned. So your next suggestion will be to not allow 
> > copy-and-move because it does not fit with the cert rule? So I think it's 
> > better to have this not a cert check then, but a bugprone check. I prefer 
> > to have a working check then implementing a theoretical documentation.
> > 
> > Apart from that cert thing, it actually seems a good idea to add a config 
> > option to allow the user to get all catches, not just the "suspicious ones".
> > It's starting to be too much for me.
> 
> It can be tricky to get this stuff right; I'm sorry the difficulties are 
> aggravating.
> 
> > First, the problem was false positives. If there are too many false 
> > positives then better to have it in the cert module.
> > Now your problem is that this is not fit with the CERT requirements, 
> > because of this matcher which reduces the false positives. 
> > Adding this check to the CERT module was not my idea in the first place. So 
> > I suggest to have it a simple bugprone check (and remove the cert alias) 
> > and also we can mention that it is related to the corresponding cert rule 
> > (but it does not conform with it entirely).
> 
> We typically ask authors to support the various coding standard modules when 
> plausible because of the considerable amount of overlap between the 
> functionalities. I don't particularly like the idea of ignoring the CERT 
> coding standard here given that the solution is almost trivial to implement. 
> However, if you want to remove it from the CERT module and not support it, 
> that's your choice.
> 
> > This check allows the usage of copy-and-move pattern, which does not 
> > conform with the cert specification either where only the self-check and 
> > copy-and-swap is mentioned.
> 
> I don't get that from my reading of the CERT rule, where it says, "A 
> user-provided copy assignment operator must prevent self-copy assignment from 
> leaving the object in an indeterminate state. This can be accomplished by 
> self-assignment tests, copy-and-swap, or other idiomatic design patterns.". 
> Copy-and-move is another idiomatic design pattern for dealing with this, and 
> I'm glad your check incorporates it. (tbh, it would be nice for the CERT rule 
> to have a compliant solution demonstrating it -- I'll recommend it on their 
> rule.)
> 
> > So your next suggestion will be to not allow copy-and-move because it does 
> > not fit with the cert rule? So I think it's better to have this not a cert 
> > check then, but a bugprone check. I prefer to have a working check then 
> > implementing a theoretical documentation.
> 
> Theoretical documentation? The CERT standard is a published standard used in 
> industry that's supported by other analyzers as well as clang-tidy, so it's 
> not really theoretical.
> 
> > Apart from that cert thing, it actually seems a good idea to add a config 
> > 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:54
+  recordType(hasDeclaration(classTemplateSpecializationDecl(
+  hasAnyName("std::shared_ptr", "std::unique_ptr", "std::weak_ptr",
+ "std::auto_ptr"),

ztamas wrote:
> aaron.ballman wrote:
> > These should be using `::std::whatever` to ensure we only care about names 
> > in the global namespace named `std`.
> > 
> > Should this list be configurable? For instance, boost has a fair number of 
> > smart pointers.
> > Should this list be configurable? For instance, boost has a fair number of 
> > smart pointers.
> 
> xazax.hun has the same suggestion. I think it's a good idea for a follow-up 
> patch. I actually added a test case with the name CustomPtrField to document 
> this future possibility.
Yeah, I think it's fine to leave the list until a follow-up patch (though the 
names should still be corrected for this patch).



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:62-64
+  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+  has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
+  hasType(arrayType(;

ztamas wrote:
> aaron.ballman wrote:
> > Hmm, while I understand why you're doing this, I'm worried that it will 
> > miss some pretty important cases. For instance, improper thread locking 
> > could result in deadlocks, improper releasing of non-memory resources could 
> > be problematic (such as network connections, file streams, etc), even 
> > simple integer assignments could be problematic in theory:
> > ```
> > Yobble& Yobble::operator=(const Yobble ) {
> >   superSecretHashVal = 0; // Being secure!
> >   ... some code that may early return ...
> >   superSecretHashVal = Y.superSecretHashVal;
> > }
> > ```
> > I wonder whether we want an option here to allow users to diagnose 
> > regardless of whether we think it's suspicious or not.
> > 
> > At the very least, this code should not be enabled for the CERT version of 
> > the check as it doesn't conform to the CERT requirements.
> It's starting to be too much for me.
> First, the problem was false positives. If there are too many false positives 
> then better to have it in the cert module.
> Now your problem is that this is not fit with the CERT requirements, because 
> of this matcher which reduces the false positives. Adding this check to the 
> CERT module was not my idea in the first place. So I suggest to have it a 
> simple bugprone check  (and remove the cert alias) and also we can mention 
> that it is related to the corresponding cert rule (but it does not conform 
> with it entirely).
> This check allows the usage of copy-and-move pattern, which does not conform 
> with the cert specification either where only the self-check and 
> copy-and-swap is mentioned. So your next suggestion will be to not allow 
> copy-and-move because it does not fit with the cert rule? So I think it's 
> better to have this not a cert check then, but a bugprone check. I prefer to 
> have a working check then implementing a theoretical documentation.
> 
> Apart from that cert thing, it actually seems a good idea to add a config 
> option to allow the user to get all catches, not just the "suspicious ones".
> It's starting to be too much for me.

It can be tricky to get this stuff right; I'm sorry the difficulties are 
aggravating.

> First, the problem was false positives. If there are too many false positives 
> then better to have it in the cert module.
> Now your problem is that this is not fit with the CERT requirements, because 
> of this matcher which reduces the false positives. 
> Adding this check to the CERT module was not my idea in the first place. So I 
> suggest to have it a simple bugprone check (and remove the cert alias) and 
> also we can mention that it is related to the corresponding cert rule (but it 
> does not conform with it entirely).

We typically ask authors to support the various coding standard modules when 
plausible because of the considerable amount of overlap between the 
functionalities. I don't particularly like the idea of ignoring the CERT coding 
standard here given that the solution is almost trivial to implement. However, 
if you want to remove it from the CERT module and not support it, that's your 
choice.

> This check allows the usage of copy-and-move pattern, which does not conform 
> with the cert specification either where only the self-check and 
> copy-and-swap is mentioned.

I don't get that from my reading of the CERT rule, where it says, "A 
user-provided copy assignment operator must prevent self-copy assignment from 
leaving the object in an indeterminate state. This can be accomplished by 
self-assignment tests, copy-and-swap, or other idiomatic design patterns.". 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas added a comment.

Ok, so I removed the alias from cert module and added CERT rule link as a "see 
also".
So I think we solved the problem that things do not conform with the CERT 
requirements and can focus on the actual problem.

Summary, templates are still ignored. If there is any idea how to solve this 
for templates easily or there is any sample code for this, then I happy to 
implement template support.
I see a related comment in clang-tidy/modernize/PassByValueCheck.cpp:

  // Clang builds a CXXConstructExpr only when it knows which
  // constructor will be called. In dependent contexts a
  // ParenListExpr is generated instead of a CXXConstructExpr,
  // filtering out templates automatically for us.

So It's not only me who sees that template AST is different.

Also, two good ideas were mentioned about extra options during the review.

1. Allow finding catches not just pointers and arrays
2. Allow configuring list of suspicious field types / pointer types

I think these two options are good ideas for future improvements. For now, the 
check seems useful and produces not many false positives in the current form, I 
think.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 5 inline comments as done.
ztamas added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-36
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant(
+  binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ has(ignoringParenCasts(cxxThisExpr()));

ztamas wrote:
> aaron.ballman wrote:
> > Will this also match code like:
> > ```
> > Frobble ::operator=(const Frobble ) {
> >   if ( == this->whatever())
> > return *this;
> > }
> > ```
> > or, more insidiously: 
> > ```
> > Frobble ::operator=(const Frobble ) {
> >   if ( == whatever()) // whatever is a member function of Frobble
> > return *this;
> > }
> > ```
> > 
> I'll check that case. The original hasLHS(...), hasRHS(...) might work with 
> this use case too.
I added NotSelfCheck test case for this, which works correctly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 196279.
ztamas added a comment.

Added missing fullstop
Added a new test cases making sure that HasNoSelfCheck works correctly
Added template instantiation to tests
Remove the alias from cert module and add the CERT link as a see also


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,545 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+class NotSelfCheck {
+public:
+  NotSelfCheck =(const NotSelfCheck ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+if ( == this->doSomething()) {
+  // ...
+}
+return *this;
+  }
+
+  void* doSomething() {
+  return p;
+  }
+

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked an inline comment as done.
ztamas added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+  int *p;
+};

aaron.ballman wrote:
> ztamas wrote:
> > aaron.ballman wrote:
> > > ztamas wrote:
> > > > JonasToth wrote:
> > > > > Please add tests with templated classes and self-assignment.
> > > > I tested with template classes, but TemplateCopyAndSwap test case, for 
> > > > example, was wrongly caught by the check. So I just changed the code to 
> > > > ignore template classes. I did not find any template class related 
> > > > catch either in LibreOffice or LLVM when I run the first version of 
> > > > this check, so we don't lose too much with ignoring template classes, I 
> > > > think.
> > > I am not keen on ignoring template classes for the check; that seems like 
> > > a bug in the check that should be addressed. At a minimum, the test cases 
> > > should be present with FIXME comments about the unwanted diagnostics.
> > I don't think it's a good idea to change the check now to catch template 
> > classes and produce false positives.
> > 
> > First of all the check does not work with template classes because the AST 
> > is different. Check TemplateCopyAndSwap test case for example. It's 
> > expected that the definition of operator= contains a CXXConstructExpr, but 
> > something is different for templates. It might be a lower level problem, 
> > how to detect a constructor call in a template class or templates just need 
> > different matcher. So implementing this check with correct template 
> > handling might be tricky and it might make the checker more complex. I'm 
> > not sure it worth the time, because as I mentioned I did not see any 
> > template related catch in the tested code bases. However, it might be a 
> > good idea to mention this limitation in the documentation.
> > 
> > About the false positives. This template related false positives are 
> > different from other false positives. In general, check doesn't warn, if 
> > the code uses one of the three patterns (self-check, copy-and-move, 
> > copy-and-swap). However, TemplateCopyAndSwap test case was wrongly caught 
> > by the check even though it uses copy-and-swap method. I would not 
> > introduce this kind of false positives. So the user of the check can expect 
> > that if he / she uses one of the three patterns, then there will be no 
> > warning in his / her code.
> > 
> > I already added five template related test cases. I think the comments are 
> > clear about which test case should be ignored by the check and which test 
> > cases are incorrectly ignored by now.
> > So implementing this check with correct template handling might be tricky 
> > and it might make the checker more complex. 
> 
> I would be surprised if it added that much complexity. You wouldn't be 
> checking the template declarations, but the template instantiations 
> themselves, which should have the same AST representation as similar 
> non-templated code.
> 
> >  I'm not sure it worth the time, because as I mentioned I did not see any 
> > template related catch in the tested code bases.
> 
> It's needed to conform to the CERT coding standard, which has no exceptions 
> for templates here.
> 
> > However, it might be a good idea to mention this limitation in the 
> > documentation.
> 
> My preference is to support it from the start, but if we don't support it, it 
> should definitely be mentioned in the documentation.
I added instatiation of template classes to the test code (locally):

```
template 
class TemplateCopyAndMove {
public:
  TemplateCopyAndMove =(const TemplateCopyAndMove ) {
TemplateCopyAndMove temp(object);
*this = std::move(temp);
return *this;
  }

private:
  T *p;
};

int instaniateTemplateCopyAndMove() {
TemplateCopyAndMove x;
(void) x;
}
```

However I don't see the expected AST neither in the ClassTemplateDecl or in the 
ClassTemplateSpecializationDecl. So how can I get that AST which is similar to 
non-template case?

```
|-ClassTemplateDecl 0x117ed20  line:342:7 
TemplateCopyAndMove
| |-TemplateTypeParmDecl 0x117ec08  col:17 referenced 
class depth 0 index 0 T
| |-CXXRecordDecl 0x117ec90  line:342:7 class 
TemplateCopyAndMove definition
| | |-DefinitionData standard_layout
| | | |-DefaultConstructor exists trivial needs_implicit
| | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
| | | |-MoveConstructor
| | | |-CopyAssignment non_trivial has_const_param user_declared 
implicit_has_const_param
| | | |-MoveAssignment
| | | `-Destructor simple irrelevant trivial needs_implicit
| | |-CXXRecordDecl 0x117ef70  col:7 implicit class 
TemplateCopyAndMove
| | |-AccessSpecDecl 0x117f000  col:1 public
| | |-CXXMethodDecl 0x117f9d0  line:344:27 operator= 
'TemplateCopyAndMove &(const TemplateCopyAndMove &)'
| | | |-ParmVarDecl 0x117f820  col:67 referenced object 'const 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 4 inline comments as done.
ztamas added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-36
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant(
+  binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ has(ignoringParenCasts(cxxThisExpr()));

aaron.ballman wrote:
> Will this also match code like:
> ```
> Frobble ::operator=(const Frobble ) {
>   if ( == this->whatever())
> return *this;
> }
> ```
> or, more insidiously: 
> ```
> Frobble ::operator=(const Frobble ) {
>   if ( == whatever()) // whatever is a member function of Frobble
> return *this;
> }
> ```
> 
I'll check that case. The original hasLHS(...), hasRHS(...) might work with 
this use case too.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:54
+  recordType(hasDeclaration(classTemplateSpecializationDecl(
+  hasAnyName("std::shared_ptr", "std::unique_ptr", "std::weak_ptr",
+ "std::auto_ptr"),

aaron.ballman wrote:
> These should be using `::std::whatever` to ensure we only care about names in 
> the global namespace named `std`.
> 
> Should this list be configurable? For instance, boost has a fair number of 
> smart pointers.
> Should this list be configurable? For instance, boost has a fair number of 
> smart pointers.

xazax.hun has the same suggestion. I think it's a good idea for a follow-up 
patch. I actually added a test case with the name CustomPtrField to document 
this future possibility.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:62-64
+  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+  has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
+  hasType(arrayType(;

aaron.ballman wrote:
> Hmm, while I understand why you're doing this, I'm worried that it will miss 
> some pretty important cases. For instance, improper thread locking could 
> result in deadlocks, improper releasing of non-memory resources could be 
> problematic (such as network connections, file streams, etc), even simple 
> integer assignments could be problematic in theory:
> ```
> Yobble& Yobble::operator=(const Yobble ) {
>   superSecretHashVal = 0; // Being secure!
>   ... some code that may early return ...
>   superSecretHashVal = Y.superSecretHashVal;
> }
> ```
> I wonder whether we want an option here to allow users to diagnose regardless 
> of whether we think it's suspicious or not.
> 
> At the very least, this code should not be enabled for the CERT version of 
> the check as it doesn't conform to the CERT requirements.
It's starting to be too much for me.
First, the problem was false positives. If there are too many false positives 
then better to have it in the cert module.
Now your problem is that this is not fit with the CERT requirements, because of 
this matcher which reduces the false positives. Adding this check to the CERT 
module was not my idea in the first place. So I suggest to have it a simple 
bugprone check  (and remove the cert alias) and also we can mention that it is 
related to the corresponding cert rule (but it does not conform with it 
entirely).
This check allows the usage of copy-and-move pattern, which does not conform 
with the cert specification either where only the self-check and copy-and-swap 
is mentioned. So your next suggestion will be to not allow copy-and-move 
because it does not fit with the cert rule? So I think it's better to have this 
not a cert check then, but a bugprone check. I prefer to have a working check 
then implementing a theoretical documentation.

Apart from that cert thing, it actually seems a good idea to add a config 
option to allow the user to get all catches, not just the "suspicious ones".



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+  diag(MatchedDecl->getLocation(),
+   "operator=() might not handle self-assignment properly");
+}

aaron.ballman wrote:
> Hmm, the "might not" seems a bit flat to me. How about: `'operator=()' does 
> not properly test for self-assignment`?
> 
> Also, do we want to have a fix-it to insert a check for self assignment at 
> the start of the function?
I don't think "test for self-assignment" will be good, because it's only one 
way to make the operator self assignment safe. In case of copy-and-swap and 
copy-and-move there is no testing of self assignment, but swaping/moving works 
in all cases without explicit self check.

A fix-it can be a good idea for a follow-up patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-36
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant(
+  binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ has(ignoringParenCasts(cxxThisExpr()));

Will this also match code like:
```
Frobble ::operator=(const Frobble ) {
  if ( == this->whatever())
return *this;
}
```
or, more insidiously: 
```
Frobble ::operator=(const Frobble ) {
  if ( == whatever()) // whatever is a member function of Frobble
return *this;
}
```




Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:54
+  recordType(hasDeclaration(classTemplateSpecializationDecl(
+  hasAnyName("std::shared_ptr", "std::unique_ptr", "std::weak_ptr",
+ "std::auto_ptr"),

These should be using `::std::whatever` to ensure we only care about names in 
the global namespace named `std`.

Should this list be configurable? For instance, boost has a fair number of 
smart pointers.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:61
+  // pointer member, then trying to access the object pointed by the pointer, 
or
+  // memcpy overlapping arrays)
+  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(

Missing a full stop at the end of the comment.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:62-64
+  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+  has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
+  hasType(arrayType(;

Hmm, while I understand why you're doing this, I'm worried that it will miss 
some pretty important cases. For instance, improper thread locking could result 
in deadlocks, improper releasing of non-memory resources could be problematic 
(such as network connections, file streams, etc), even simple integer 
assignments could be problematic in theory:
```
Yobble& Yobble::operator=(const Yobble ) {
  superSecretHashVal = 0; // Being secure!
  ... some code that may early return ...
  superSecretHashVal = Y.superSecretHashVal;
}
```
I wonder whether we want an option here to allow users to diagnose regardless 
of whether we think it's suspicious or not.

At the very least, this code should not be enabled for the CERT version of the 
check as it doesn't conform to the CERT requirements.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+  diag(MatchedDecl->getLocation(),
+   "operator=() might not handle self-assignment properly");
+}

Hmm, the "might not" seems a bit flat to me. How about: `'operator=()' does not 
properly test for self-assignment`?

Also, do we want to have a fix-it to insert a check for self assignment at the 
start of the function?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6
+
+`cert-oop54-cpp` redirects here as an alias for this check.
+

ztamas wrote:
> aaron.ballman wrote:
> > You should add a link to CERT's documentation somewhere around this text.
> In an earlier version of this patch, there was a link. I removed it because 
> of a reviewer comment. Now I add it back, I hope now are OK.
Thank you for adding the link!



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+  int *p;
+};

ztamas wrote:
> aaron.ballman wrote:
> > ztamas wrote:
> > > JonasToth wrote:
> > > > Please add tests with templated classes and self-assignment.
> > > I tested with template classes, but TemplateCopyAndSwap test case, for 
> > > example, was wrongly caught by the check. So I just changed the code to 
> > > ignore template classes. I did not find any template class related catch 
> > > either in LibreOffice or LLVM when I run the first version of this check, 
> > > so we don't lose too much with ignoring template classes, I think.
> > I am not keen on ignoring template classes for the check; that seems like a 
> > bug in the check that should be addressed. At a minimum, the test cases 
> > should be present with FIXME comments about the unwanted diagnostics.
> I don't think it's a good idea to change the check now to catch template 
> classes and produce false positives.
> 
> First of all the check does not work with template classes because the AST is 
> different. Check TemplateCopyAndSwap test case for example. It's expected 
> that the definition of operator= contains a CXXConstructExpr, but something 
> is different for templates. It might be a lower level problem, how to detect 
> a constructor 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 196066.
ztamas added a comment.

Add false positive test cases.
Added a note about template related limitation to the docs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,502 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+///
+/// Test cases correctly ignored by the check.
+
+// Self-assignment is checked using the equality operator.
+class SelfCheck1 {
+public:
+  SelfCheck1 =(const SelfCheck1 ) {
+if (this == )
+  return *this;
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class SelfCheck2 {
+public:
+  SelfCheck2 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 2 inline comments as done.
ztamas added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+  int *p;
+};

aaron.ballman wrote:
> ztamas wrote:
> > JonasToth wrote:
> > > Please add tests with templated classes and self-assignment.
> > I tested with template classes, but TemplateCopyAndSwap test case, for 
> > example, was wrongly caught by the check. So I just changed the code to 
> > ignore template classes. I did not find any template class related catch 
> > either in LibreOffice or LLVM when I run the first version of this check, 
> > so we don't lose too much with ignoring template classes, I think.
> I am not keen on ignoring template classes for the check; that seems like a 
> bug in the check that should be addressed. At a minimum, the test cases 
> should be present with FIXME comments about the unwanted diagnostics.
I don't think it's a good idea to change the check now to catch template 
classes and produce false positives.

First of all the check does not work with template classes because the AST is 
different. Check TemplateCopyAndSwap test case for example. It's expected that 
the definition of operator= contains a CXXConstructExpr, but something is 
different for templates. It might be a lower level problem, how to detect a 
constructor call in a template class or templates just need different matcher. 
So implementing this check with correct template handling might be tricky and 
it might make the checker more complex. I'm not sure it worth the time, because 
as I mentioned I did not see any template related catch in the tested code 
bases. However, it might be a good idea to mention this limitation in the 
documentation.

About the false positives. This template related false positives are different 
from other false positives. In general, check doesn't warn, if the code uses 
one of the three patterns (self-check, copy-and-move, copy-and-swap). However, 
TemplateCopyAndSwap test case was wrongly caught by the check even though it 
uses copy-and-swap method. I would not introduce this kind of false positives. 
So the user of the check can expect that if he / she uses one of the three 
patterns, then there will be no warning in his / her code.

I already added five template related test cases. I think the comments are 
clear about which test case should be ignored by the check and which test cases 
are incorrectly ignored by now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 4 inline comments as done.
ztamas added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6
+
+`cert-oop54-cpp` redirects here as an alias for this check.
+

aaron.ballman wrote:
> You should add a link to CERT's documentation somewhere around this text.
In an earlier version of this patch, there was a link. I removed it because of 
a reviewer comment. Now I add it back, I hope now are OK.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 195382.
ztamas added a comment.

Update patch based on reviewer comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,434 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+///
+/// Test cases correctly ignored by the check.
+
+// Self-assignment is checked using the equality operator.
+class SelfCheck1 {
+public:
+  SelfCheck1 =(const SelfCheck1 ) {
+if (this == )
+  return *this;
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class SelfCheck2 {
+public:
+  SelfCheck2 =(const SelfCheck2 ) {
+if ( == this)
+  

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:130-131
 "bugprone-throw-keyword-missing");
+CheckFactories.registerCheck(
+"bugprone-too-small-loop-variable");
 CheckFactories.registerCheck(

This should be done in a separate patch, as it's unrelated to this patch.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:19
+
+void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
+  // We don't care about deleted, default or implicit operator implementations.

You should only register these matchers when in C++ mode.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10
+
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment

ztamas wrote:
> aaron.ballman wrote:
> > Eugene.Zelenko wrote:
> > > alexfh wrote:
> > > > JonasToth wrote:
> > > > > It is worth an alias into the cert module. Please add this with this 
> > > > > revision already.
> > > > Please add a corresponding alias into the cert module.
> > > See also other aliased checks for documentation examples.
> > I kind of wonder whether we want it as an alias in the CERT module or just 
> > move it into the CERT module directly (and maybe provide an alias to 
> > bugprone, if we think the fp rate is reasonable enough).
> Now, I just added a cert alias. I can move this check to cert module entirely 
> if needed. In general, I prefer a meaningful name over a cert number, that's 
> why it might be more useful to have also a bugprone prefixed check for this. 
> And it seems to me cert checks used to be the aliases.
If the purpose to the check is to conform to a specific coding standard, we 
usually put the check with the coding standard it supports unless the check is 
generally useful (then we put it into a module and alias into the coding 
standard module). I'm having a hard time convincing myself which way to go in 
this case, so I guess that means I don't have a strong opinion. :-)



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6
+
+`cert-oop54-cpp` redirects here as an alias for this check.
+

You should add a link to CERT's documentation somewhere around this text.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+  int *p;
+};

ztamas wrote:
> JonasToth wrote:
> > Please add tests with templated classes and self-assignment.
> I tested with template classes, but TemplateCopyAndSwap test case, for 
> example, was wrongly caught by the check. So I just changed the code to 
> ignore template classes. I did not find any template class related catch 
> either in LibreOffice or LLVM when I run the first version of this check, so 
> we don't lose too much with ignoring template classes, I think.
I am not keen on ignoring template classes for the check; that seems like a bug 
in the check that should be addressed. At a minimum, the test cases should be 
present with FIXME comments about the unwanted diagnostics.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:326
+
+// We should not catch move assignment operators.
+class MoveAssignOperator {

While I do agree move assignment operators should not be checked by default 
some would argue that move assignments should still be tolerant to self 
assignments as one could write something like:
```
x = std::move(aliasToX);
```
Having an option to also check move assignments in a follow-up patch would be 
also nice to have.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked an inline comment as done.
ztamas added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:51
+  // Matcher for standard smart pointers.
+  const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(classTemplateSpecializationDecl(

xazax.hun wrote:
> JonasToth wrote:
> > what about `auto_ptr`? I am actually not sure if we should care, as its 
> > deprecated and removed already, on the other hand legacy code probably 
> > still has it.
> I am perfectly fine with addressing this in a follow-up patch or even not 
> addressing at all, but I think for some users it might be useful to be able 
> to specify a set of suspicious types as a configuration option such as custom 
> smart pointers, handles and so on. 
Seems a good idea for a follow-up change.
Actually, with an earlier version of this patch I found another vulnerable copy 
assignment operator in llvm code which uses a custom pointer type 
(PointerIntPair):
FunctionInfo =(const FunctionInfo )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:51
+  // Matcher for standard smart pointers.
+  const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(classTemplateSpecializationDecl(

JonasToth wrote:
> what about `auto_ptr`? I am actually not sure if we should care, as its 
> deprecated and removed already, on the other hand legacy code probably still 
> has it.
I am perfectly fine with addressing this in a follow-up patch or even not 
addressing at all, but I think for some users it might be useful to be able to 
specify a set of suspicious types as a configuration option such as custom 
smart pointers, handles and so on. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 195061.
ztamas added a comment.

Missed to syncronize ReleasNotes with the corresponding doc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,434 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+///
+/// Test cases correctly ignored by the check.
+
+// Self-assignment is checked using the equality operator.
+class SelfCheck1 {
+public:
+  SelfCheck1 =(const SelfCheck1 ) {
+if (this == )
+  return *this;
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class SelfCheck2 {
+public:
+  SelfCheck2 =(const SelfCheck2 ) {
+if 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 194925.
ztamas marked an inline comment as done.
ztamas added a comment.

Documentation fixes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,434 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+///
+/// Test cases correctly ignored by the check.
+
+// Self-assignment is checked using the equality operator.
+class SelfCheck1 {
+public:
+  SelfCheck1 =(const SelfCheck1 ) {
+if (this == )
+  return *this;
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class SelfCheck2 {
+public:
+  SelfCheck2 =(const SelfCheck2 ) {
+if ( == this)
+  return *this;
+

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 13 inline comments as done.
ztamas added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10
+
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment

aaron.ballman wrote:
> Eugene.Zelenko wrote:
> > alexfh wrote:
> > > JonasToth wrote:
> > > > It is worth an alias into the cert module. Please add this with this 
> > > > revision already.
> > > Please add a corresponding alias into the cert module.
> > See also other aliased checks for documentation examples.
> I kind of wonder whether we want it as an alias in the CERT module or just 
> move it into the CERT module directly (and maybe provide an alias to 
> bugprone, if we think the fp rate is reasonable enough).
Now, I just added a cert alias. I can move this check to cert module entirely 
if needed. In general, I prefer a meaningful name over a cert number, that's 
why it might be more useful to have also a bugprone prefixed check for this. 
And it seems to me cert checks used to be the aliases.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+  int *p;
+};

JonasToth wrote:
> Please add tests with templated classes and self-assignment.
I tested with template classes, but TemplateCopyAndSwap test case, for example, 
was wrongly caught by the check. So I just changed the code to ignore template 
classes. I did not find any template class related catch either in LibreOffice 
or LLVM when I run the first version of this check, so we don't lose too much 
with ignoring template classes, I think.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6
+
+"cert-oop54-cpp" redirects here as an alias for this check.
+

Please use back-tick to highlight cert-oop54-cpp.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:12
+
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment

Should be removed, since it's in cert-oop54-cpp.rst now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 194921.
ztamas added a comment.

Updated the code based on reviewer comments:

- Alphabetical order in `BugproneTidyModule.cpp`
- Handling of `auto_ptr`
- Test cases for template classes (I made the check ignore these case otherwise 
this can lead to false positives)
- Added cert alias
- Simplified the matcher as requested (`hasAnyName()`, 
`has(ignoringParenCasts(cxxThisExpr())`
- Remove empty lines and synchronized the corresponding comment with the 
Release Notes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,434 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+///
+/// Test cases 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas added a comment.

I'll update the patch based on the comments.
Just a note about the false positive ratio. In LibreOffice we run other static 
analyzers too, that's why there were less positive catches there than false 
positives. For example, PVS studio also catches unprotected copy assignment 
operators. Some of these issues were already fixed before I run this new check 
on the code:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=e5e0cc68f70d35e1849aeaf21c0ce68afd6a1f59


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10
+
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment

Eugene.Zelenko wrote:
> alexfh wrote:
> > JonasToth wrote:
> > > It is worth an alias into the cert module. Please add this with this 
> > > revision already.
> > Please add a corresponding alias into the cert module.
> See also other aliased checks for documentation examples.
I kind of wonder whether we want it as an alias in the CERT module or just move 
it into the CERT module directly (and maybe provide an alias to bugprone, if we 
think the fp rate is reasonable enough).


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:20
+void UnhandledSelfAssignmentCheck::registerMatchers(MatchFinder *Finder) {
+
+  // We don't care about deleted, default or implicit operator implementations.

Unnecessary empty line.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:76
+const MatchFinder::MatchResult ) {
+
+  const auto *MatchedDecl =

Unnecessary empty line.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:6
+
+Finds user-defined copy assignment operators which do not protect the code
+against self-assignment either by checking self-assignment explicitly or

Please synchronize this statement with Release Notes.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10-12
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment
+`_.

alexfh wrote:
> JonasToth wrote:
> > It is worth an alias into the cert module. Please add this with this 
> > revision already.
> Please add a corresponding alias into the cert module.
See also other aliased checks for documentation examples.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:41
+
+
+There are two common C++ patterns to avoid this problem. The first is

Unnecessary empty line.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:66
+
+
+The second one is the copy-and-swap method when we create a temporary copy

Unnecessary empty line.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Thanks for the useful check! I have a few comments, see inline.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-35
+  binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ anyOf(hasLHS(ignoringParenCasts(cxxThisExpr())),
+   hasRHS(ignoringParenCasts(cxxThisExpr(;
+

I guess, `has(ignoringParenCasts(cxxThisExpr())` will have the same effect as 
`anyOf(hasLHS(...), hasRHS(...))`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:53
+  recordType(hasDeclaration(classTemplateSpecializationDecl(
+  anyOf(hasName("std::shared_ptr"), hasName("std::unique_ptr"),
+hasName("std::weak_ptr")),

Please use hasAnyName. It's more efficient and readable.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10-12
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment
+`_.

JonasToth wrote:
> It is worth an alias into the cert module. Please add this with this revision 
> already.
Please add a corresponding alias into the cert module.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:102
 "bugprone-too-small-loop-variable");
+CheckFactories.registerCheck(
+"bugprone-unhandled-self-assignment");

please order this list alphabetically.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:51
+  // Matcher for standard smart pointers.
+  const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(classTemplateSpecializationDecl(

what about `auto_ptr`? I am actually not sure if we should care, as its 
deprecated and removed already, on the other hand legacy code probably still 
has it.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst:10
+
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP54-CPP. Gracefully handle self-copy assignment

It is worth an alias into the cert module. Please add this with this revision 
already.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:351
+  int *p;
+};

Please add tests with templated classes and self-assignment.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas added a comment.

On LibreOffice source code I found 36 catches with this check.
16 of them was worth to fix because in those cases the object state was changed 
in some way after a self-assignment:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=3a5d78365dd172881c16c03e67f2d170ffc6d7d4

The remaining 20 are false positives. They are working copy assignment 
operators without using any of the three checked patterns.
However, some of these warnings could be suppressed with modernizing the code 
or with removing code duplication:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=adfba503c792fdbd4748d6680c2dd8d8d5bb0d69


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas added a comment.

On llvm source code the check finds three suspicious methods:

  
/home/zolnai/libreoffice/clang/llvm-project/clang/lib/AST/NestedNameSpecifier.cpp:534:1:
 warning: operator=() might not handle self-assignment properly 
[bugprone-unhandled-self-assignment]
  operator=(const NestedNameSpecifierLocBuilder ) {
  
  
/home/zolnai/libreoffice/clang/llvm-project/llvm/unittests/ADT/ArrayRefTest.cpp:75:10:
 warning: operator=() might not handle self-assignment properly 
[bugprone-unhandled-self-assignment]
  void operator=(const NonAssignable ) { assert(RHS.Ptr != nullptr); }
  
  
/home/zolnai/libreoffice/clang/llvm-project/llvm/lib/Analysis/TargetLibraryInfo.cpp:594:47:
 warning: operator=() might not handle self-assignment properly 
[bugprone-unhandled-self-assignment]
  TargetLibraryInfoImpl ::operator=(const 
TargetLibraryInfoImpl ) {

Two of them seems a good catch to me. NestedNameSpecifierLocBuilder and 
TargetLibraryInfoImpl are using `memcpy` without self-check, and using `memcpy` 
on overlapping regions leads to undefined behavior.

The third one is not an actual working copy assignment operator, as I see. It 
is used only for some testing purposes.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas created this revision.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
Herald added a project: clang.

This check searches for copy assignment operators which might not handle 
self-assignment properly. There are three patterns of
handling a self assignment situation: self check, copy-and-swap or the less 
common copy-and-move. The new check warns if none of
these patterns is found in a user defined implementation.

See also:
OOP54-CPP. Gracefully handle self-copy assignment
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP54-CPP.+Gracefully+handle+self-copy+assignment


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,351 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+///
+/// Test cases correctly ignored by the check.
+
+// Self-assignment is checked using the equality operator.
+class SelfCheck1 {
+public:
+  SelfCheck1 =(const SelfCheck1 ) {
+if (this == )
+  return *this;
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class SelfCheck2 {
+public:
+  SelfCheck2 =(const