[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0f9e1e3ae750: [clang-tidy]: fix false positive of 
cert-oop54-cpp check. (authored by ztamas).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990

Files:
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -212,6 +212,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo2;
+template 
+bool operator!=(Foo2 &, Foo2 &) {
+  class Bar2 {
+Bar2 =(const Bar2 ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: operator=() does not handle 
self-assignment properly [bugprone-unhandled-self-assignment]
+  p = other.p;
+  return *this;
+}
+
+int *p;
+  };
+}
+
 ///
 /// Test cases correctly ignored by the check.
 
@@ -283,6 +298,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by 
value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -212,6 +212,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo2;
+template 
+bool operator!=(Foo2 &, Foo2 &) {
+  class Bar2 {
+Bar2 =(const Bar2 ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  p = other.p;
+  return *this;
+}
+
+int *p;
+  };
+}
+
 ///
 /// Test cases correctly ignored by the check.
 
@@ -283,6 +298,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

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

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

Add suggested test case and rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990

Files:
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -212,6 +212,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo2;
+template 
+bool operator!=(Foo2 &, Foo2 &) {
+  class Bar2 {
+Bar2 =(const Bar2 ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: operator=() does not handle 
self-assignment properly [bugprone-unhandled-self-assignment]
+  p = other.p;
+  return *this;
+}
+
+int *p;
+  };
+}
+
 ///
 /// Test cases correctly ignored by the check.
 
@@ -283,6 +298,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by 
value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -212,6 +212,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo2;
+template 
+bool operator!=(Foo2 &, Foo2 &) {
+  class Bar2 {
+Bar2 =(const Bar2 ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  p = other.p;
+  return *this;
+}
+
+int *p;
+  };
+}
+
 ///
 /// Test cases correctly ignored by the check.
 
@@ -283,6 +298,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  has(ignoringParenCasts(cxxThisExpr(;
 
   // Both copy-and-swap and copy-and-move method creates a copy 

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

[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 with a testing request. I agree that the Clang AST is a bit surprising, 
but not so surprising that I could definitely call it a bug.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp:286-299
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {

Thank you for this! Can you also add a test case where the diagnostic should 
trigger? e.g.,
```
class Foo;
template 
bool operator!=(Foo &, Foo &) {
  class Bar {
Bar =(const Bar ) {
  p = other.p;
  return *this;
}

int *p;
  };
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990



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

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

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

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

Remove false TODO comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990

Files:
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -283,6 +283,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by 
value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -283,6 +283,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D76990#1948516 , @ztamas wrote:

> I agree, it seems suspicious that a BinaryOperator matcher does not work in 
> this case. However, I'm working on this level of the code, I'm looking at the 
> matchers like an API, what I'm just using in the clang-tidy code, without 
> changing them. So that's why I added this workaround. I don't know how much 
> time it would take to fix this issue in the matcher code or whether it will 
> be fixed in the near future or not, but until then I think it useful to have 
> this workaround in the caller code, so this clang-tidy check works better. I 
> added a TODO comment, so it's more visible that we have an issue here, so 
> it's more probable somebody will fix that working with the matchers.


It's not the matchers fault, that's working as intended. its the AST that clang 
has generated for that source code that looks suspicious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990



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

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

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

ztamas added a comment.

In D76990#1948286 , @njames93 wrote:

> I'm not entirely sure this is where the fix needs to be for this. The test 
> case code is whacky as hell, but from what I can see clang should always emit 
> a `BinaryOperator` for dependent type operators. No idea why it is spewing 
> out a `CXXOperatorCallExpr` instead. Would need someone with more knowledge 
> on Sema to confirm(or deny) this though.


I agree, it seems suspicious that a BinaryOperator matcher does not work in 
this case. However, I'm working on this level of the code, I'm looking at the 
matchers like an API, what I'm just using in the clang-tidy code, without 
changing them. So that's why I added this workaround. I don't know how much 
time it would take to fix this issue in the matcher code or whether it will be 
fixed in the near future or not, but until then I think it useful to have this 
workaround in the caller code, so this clang-tidy check works better. I added a 
TODO comment, so it's more visible that we have an issue here, so it's more 
probable somebody will fix that working with the matchers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990



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

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

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

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

Rebase, TODO comment, remove unrelated change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990

Files:
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -283,6 +283,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by 
value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,14 @@
 
   // 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(
+  // TODO: Solve this on the matcher level, so we don't need to use two 
different
+  // matchers for the same thing.
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -283,6 +283,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,14 @@
 
   // 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(
+  // TODO: Solve this on the matcher level, so we don't need to use two different
+  // matchers for the same thing.
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I'm not entirely sure this is where the fix needs to be for this. The test case 
code is whacky as hell, but from what I can see clang should always emit a 
`BinaryOperator` for dependent type operators. No idea why it is spewing out a 
`CXXOperatorCallExpr` instead. Would need someone with more knowledge on Sema 
to confirm(or deny) this though.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp:516
 public:
-  NotACopyAssignmentOperator& operator=(const NotACopyAssignmentOperator ) {
+  NotACopyAssignmentOperator =(const NotACopyAssignmentOperator ) {
 Ptr1 = RHS.getUy();

clang format artefact? This change should be undone as its unrelated to the 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990



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

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

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

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

Run clang-format on test code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76990

Files:
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -283,6 +283,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by 
value.
 class PassedByValue {
 public:
@@ -498,7 +513,7 @@
   Uy *Ptr2;
 
 public:
-  NotACopyAssignmentOperator& operator=(const NotACopyAssignmentOperator ) {
+  NotACopyAssignmentOperator =(const NotACopyAssignmentOperator ) {
 Ptr1 = RHS.getUy();
 Ptr2 = RHS.getTy();
 return *this;
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -283,6 +283,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template 
+bool operator!=(Foo &, Foo &) {
+  class Bar {
+Bar =(const Bar ) {
+  if (this != ) {
+  }
+  return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by value.
 class PassedByValue {
 public:
@@ -498,7 +513,7 @@
   Uy *Ptr2;
 
 public:
-  NotACopyAssignmentOperator& operator=(const NotACopyAssignmentOperator ) {
+  NotACopyAssignmentOperator =(const NotACopyAssignmentOperator ) {
 Ptr1 = RHS.getUy();
 Ptr2 = RHS.getTy();
 return *this;
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76990: [clang-tidy]: fix false positive of cert-oop54-cpp check.

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

ztamas created this revision.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
ztamas edited the summary of this revision.
ztamas added a reviewer: aaron.ballman.

It seems we need a different matcher for binary operator
in a template context.

Fixes this issue:
https://bugs.llvm.org/show_bug.cgi?id=44499


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76990

Files:
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -283,6 +283,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template  bool operator!=(Foo&, Foo&) {
+  class Bar {
+Bar& operator=(const Bar& other)
+{
+if (this != ) {
+}
+return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by 
value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-self-assignment.cpp
@@ -283,6 +283,21 @@
   T *p;
 };
 
+// https://bugs.llvm.org/show_bug.cgi?id=44499
+class Foo;
+template  bool operator!=(Foo&, Foo&) {
+  class Bar {
+Bar& operator=(const Bar& other)
+{
+if (this != ) {
+}
+return *this;
+}
+
+int *p;
+  };
+}
+
 // There is no warning if the copy assignment operator gets the object by value.
 class PassedByValue {
 public:
Index: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
@@ -40,9 +40,12 @@
 
   // 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(
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(anyOf(
   hasDescendant(binaryOperator(hasAnyOperatorName("==", "!="),
-   has(ignoringParenCasts(cxxThisExpr()));
+   has(ignoringParenCasts(cxxThisExpr(),
+  hasDescendant(cxxOperatorCallExpr(
+  hasAnyOverloadedOperatorName("==", "!="), argumentCountIs(2),
+  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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits