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:
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:
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
