[PATCH] D122535: [clang-tidy] Never consider assignments as equivalent in `misc-redundant-expression` check

[Fabian Wolff via Phabricator via cfe-commits]

fwolff created this revision.
fwolff added reviewers: aaron.ballman, njames93.
fwolff added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
fwolff requested review of this revision.
Herald added a subscriber: cfe-commits.

Attempts to fix #35853 <https://github.com/llvm/llvm-project/issues/35853>. This issue is about side-effectful function calls, but since almost every function call potentially causes side effects and would therefore prevent any `misc-redundant-expression` warnings, I have focused on assignments instead, which would also fix the example in #35853.

We already have special treatment of unary increment/decrement here <https://github.com/llvm/llvm-project/blob/34b547dfbf7660575b815e39de5bb043857579ca/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp#L132>, even though this can also lead to false negatives. Replacing `i++` with `i = i + 1` currently changes the behavior of the `misc-redundant-expression` check, whereas the change proposed here prevents any warnings about redundant expressions if assignments are involved. This can cause false negatives, though (whereas the current implementation causes false positives), so let me know whether you think this is a sensible change to make.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122535

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -831,3 +831,15 @@
 };
 
 } // namespace no_crash
+
+int TestAssignSideEffect(int i) {
+  int k = i;
+
+  if ((k = k + 1) != 1 || (k = k + 1) != 2)
+    return 0;
+
+  if ((k = foo(0)) != 1 || (k = foo(0)) != 2)
+    return 1;
+
+  return 2;
+}
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -134,6 +134,8 @@
     return cast<UnaryOperator>(Left)->getOpcode() ==
            cast<UnaryOperator>(Right)->getOpcode();
   case Stmt::BinaryOperatorClass:
+    if (cast<BinaryOperator>(Left)->isAssignmentOp())
+      return false;
     return cast<BinaryOperator>(Left)->getOpcode() ==
            cast<BinaryOperator>(Right)->getOpcode();
   case Stmt::UnaryExprOrTypeTraitExprClass:


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122535.418420.patch
Type: text/x-patch
Size: 1213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220326/7b2e0f78/attachment-0001.bin>