[clang-tools-extra] [clang-tidy] bugprone-assert-side-effect non-const operator methods (PR #71974)

via cfe-commits cfe-commits at lists.llvm.org
Wed Nov 15 11:09:17 PST 2023


https://github.com/schenker updated https://github.com/llvm/llvm-project/pull/71974

>From dabfdee1a982000605e4b33930ba433c63d1684f Mon Sep 17 00:00:00 2001
From: Thomas Schenker <thomas.schenker at protonmail.com>
Date: Fri, 10 Nov 2023 18:58:26 +0100
Subject: [PATCH 1/4] [clang-tidy] bugprone-assert-side-effect non-const
 operator methods

With this PR, `bugprone-assert-side-effect` assumes that operator methods
that are not marked as `const` have side effects. This matches the
existing rule for non-operator methods.

E.g. the following snippet is now reported and was previously not:
```
std::stringstream ss;
assert(ss << 1);
```
---
 .../clang-tidy/bugprone/AssertSideEffectCheck.cpp   |  4 ++++
 .../checkers/bugprone/assert-side-effect.cpp        | 13 ++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
index 07a987359d4d8d7..599f5ac70e7a229 100644
--- a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
@@ -41,6 +41,10 @@ AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls,
   }
 
   if (const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
+    if (const auto *FuncDecl = OpCallExpr->getDirectCallee())
+      if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl))
+        return !MethodDecl->isConst();
+
     OverloadedOperatorKind OpKind = OpCallExpr->getOperator();
     return OpKind == OO_Equal || OpKind == OO_PlusEqual ||
            OpKind == OO_MinusEqual || OpKind == OO_StarEqual ||
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
index 6c41e1e320adeac..49d3d456deb9d35 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
@@ -11,7 +11,7 @@ class MyClass {
 
   MyClass &operator=(const MyClass &rhs) { return *this; }
 
-  int operator-() { return 1; }
+  int operator-() const { return 1; }
 
   operator bool() const { return true; }
 
@@ -84,5 +84,16 @@ int main() {
   msvc_assert(mc2 = mc);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in msvc_assert() condition discarded in release builds
 
+  struct {
+    int operator<<(int i) const { return i; }
+  } constOp;
+  assert(constOp << 1);
+
+  struct {
+    int operator<<(int i) { return i; }
+  } nonConstOp;
+  assert(nonConstOp << 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+
   return 0;
 }

>From 071e24c78d551301b14e7bbeebd7d1f36466a8e7 Mon Sep 17 00:00:00 2001
From: Thomas Schenker <thomas.schenker at protonmail.com>
Date: Fri, 10 Nov 2023 21:07:39 +0100
Subject: [PATCH 2/4] use dyn_cast_or_null and add release note

---
 .../clang-tidy/bugprone/AssertSideEffectCheck.cpp           | 6 +++---
 clang-tools-extra/docs/ReleaseNotes.rst                     | 4 ++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
index 599f5ac70e7a229..a6c01704b3d909f 100644
--- a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
@@ -41,9 +41,9 @@ AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls,
   }
 
   if (const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
-    if (const auto *FuncDecl = OpCallExpr->getDirectCallee())
-      if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl))
-        return !MethodDecl->isConst();
+    if (const auto *MethodDecl =
+            dyn_cast_or_null<CXXMethodDecl>(OpCallExpr->getDirectCallee()))
+      return !MethodDecl->isConst();
 
     OverloadedOperatorKind OpKind = OpCallExpr->getOperator();
     return OpKind == OO_Equal || OpKind == OO_PlusEqual ||
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index c79aafdc0f06e69..846a029810454f1 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -208,6 +208,10 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`bugprone-assert-side-effect
+  <clang-tidy/checks/bugprone/assert-side-effect>` check to report usage of
+  non-const operator methods in assertions.
+
 - Improved :doc:`bugprone-dangling-handle
   <clang-tidy/checks/bugprone/dangling-handle>` check to support functional
   casting during type conversions at variable initialization, now with improved

>From 68048c10877eec6b9ae86c050d1a406b2c0f7cea Mon Sep 17 00:00:00 2001
From: Thomas Schenker <thomas.schenker at protonmail.com>
Date: Fri, 10 Nov 2023 21:45:52 +0100
Subject: [PATCH 3/4] test const overload

---
 .../checkers/bugprone/assert-side-effect.cpp      | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
index 49d3d456deb9d35..c39cb422ebc68cc 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
@@ -84,15 +84,16 @@ int main() {
   msvc_assert(mc2 = mc);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in msvc_assert() condition discarded in release builds
 
-  struct {
+  struct ConstOverload {
     int operator<<(int i) const { return i; }
-  } constOp;
-  assert(constOp << 1);
-
-  struct {
     int operator<<(int i) { return i; }
-  } nonConstOp;
-  assert(nonConstOp << 1);
+  };
+
+  const ConstOverload const_instance;
+  assert(const_instance << 1);
+
+  ConstOverload mutable_instance;
+  assert(mutable_instance << 1);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
 
   return 0;

>From 2494d69c42cdbbda6c235f70215ef8c452861304 Mon Sep 17 00:00:00 2001
From: Thomas Schenker <thomas.schenker at protonmail.com>
Date: Mon, 13 Nov 2023 21:54:03 +0100
Subject: [PATCH 4/4] always consider const operator calls side-effect free,
 add << and >> operators to the list of operators potentially having side
 effects

---
 .../bugprone/AssertSideEffectCheck.cpp        |  4 +++-
 clang-tools-extra/docs/ReleaseNotes.rst       |  3 ++-
 .../checkers/bugprone/assert-side-effect.cpp  | 20 ++++++++++++++-----
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
index a6c01704b3d909f..43bedd4f73ef447 100644
--- a/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp
@@ -43,7 +43,8 @@ AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls,
   if (const auto *OpCallExpr = dyn_cast<CXXOperatorCallExpr>(E)) {
     if (const auto *MethodDecl =
             dyn_cast_or_null<CXXMethodDecl>(OpCallExpr->getDirectCallee()))
-      return !MethodDecl->isConst();
+      if (MethodDecl->isConst())
+        return false;
 
     OverloadedOperatorKind OpKind = OpCallExpr->getOperator();
     return OpKind == OO_Equal || OpKind == OO_PlusEqual ||
@@ -51,6 +52,7 @@ AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls,
            OpKind == OO_SlashEqual || OpKind == OO_AmpEqual ||
            OpKind == OO_PipeEqual || OpKind == OO_CaretEqual ||
            OpKind == OO_LessLessEqual || OpKind == OO_GreaterGreaterEqual ||
+           OpKind == OO_LessLess || OpKind == OO_GreaterGreater ||
            OpKind == OO_PlusPlus || OpKind == OO_MinusMinus ||
            OpKind == OO_PercentEqual || OpKind == OO_New ||
            OpKind == OO_Delete || OpKind == OO_Array_New ||
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 846a029810454f1..1da8a92a1effd04 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -210,7 +210,8 @@ Changes in existing checks
 
 - Improved :doc:`bugprone-assert-side-effect
   <clang-tidy/checks/bugprone/assert-side-effect>` check to report usage of
-  non-const operator methods in assertions.
+  non-const `<<` and `>>` operators in assertions and fixed some false-positives
+  with const operators.
 
 - Improved :doc:`bugprone-dangling-handle
   <clang-tidy/checks/bugprone/dangling-handle>` check to support functional
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
index c39cb422ebc68cc..c11638aa823aaeb 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp
@@ -11,7 +11,7 @@ class MyClass {
 
   MyClass &operator=(const MyClass &rhs) { return *this; }
 
-  int operator-() const { return 1; }
+  int operator-() { return 1; }
 
   operator bool() const { return true; }
 
@@ -84,17 +84,27 @@ int main() {
   msvc_assert(mc2 = mc);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in msvc_assert() condition discarded in release builds
 
-  struct ConstOverload {
+  struct OperatorTest {
     int operator<<(int i) const { return i; }
     int operator<<(int i) { return i; }
+    int operator+=(int i) const { return i; }
+    int operator+=(int i) { return i; }
   };
 
-  const ConstOverload const_instance;
+  const OperatorTest const_instance;
   assert(const_instance << 1);
+  assert(const_instance += 1);
 
-  ConstOverload mutable_instance;
-  assert(mutable_instance << 1);
+  OperatorTest non_const_instance;
+  assert(static_cast<const OperatorTest>(non_const_instance) << 1);
+  assert(static_cast<const OperatorTest>(non_const_instance) += 1);
+  assert(non_const_instance << 1);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+  assert(non_const_instance += 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+
+  assert(5<<1);
+  assert(5>>1);
 
   return 0;
 }



More information about the cfe-commits mailing list