-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy] detect arithmetic operations within member list initialization in modernize-use-default-member-init check #129370
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: David Rivera (RiverDave) ChangesThis aims to address a portion of #122480 by adding matchers on binary operators. This allows explicit arithmetic operations within initializers. NoteI'm aware of the other false-negatives presented in the issue above, specifically not matching explicit castings & constexpr values. These will be addressed separately on a different PR which is in the process and shouldn't take long (let me know if that's the right approach). Feedback is much appreciated. Full diff: https://github.com/llvm/llvm-project/pull/129370.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
index 6c06b0af342f6..a9d9e8b66ea7c 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
@@ -159,6 +159,12 @@ static bool sameValue(const Expr *E1, const Expr *E2) {
case Stmt::UnaryOperatorClass:
return sameValue(cast<UnaryOperator>(E1)->getSubExpr(),
cast<UnaryOperator>(E2)->getSubExpr());
+ case Stmt::BinaryOperatorClass: {
+ const auto *BinOp1 = cast<BinaryOperator>(E1);
+ const auto *BinOp2 = cast<BinaryOperator>(E2);
+ return sameValue(BinOp1->getLHS(), BinOp2->getLHS()) &&
+ sameValue(BinOp1->getRHS(), BinOp2->getRHS());
+ }
case Stmt::CharacterLiteralClass:
return cast<CharacterLiteral>(E1)->getValue() ==
cast<CharacterLiteral>(E2)->getValue();
@@ -202,7 +208,12 @@ void UseDefaultMemberInitCheck::registerMatchers(MatchFinder *Finder) {
unaryOperator(hasAnyOperatorName("+", "-"),
hasUnaryOperand(floatLiteral())),
cxxBoolLiteral(), cxxNullPtrLiteralExpr(), implicitValueInitExpr(),
- declRefExpr(to(enumConstantDecl())));
+ declRefExpr(to(enumConstantDecl())),
+ binaryOperator(
+ hasLHS(anyOf(integerLiteral(), floatLiteral(),
+ declRefExpr(to(enumConstantDecl())), binaryOperator())),
+ hasRHS(anyOf(integerLiteral(), floatLiteral(),
+ declRefExpr(to(enumConstantDecl())), binaryOperator()))));
auto Init =
anyOf(initListExpr(anyOf(allOf(initCountIs(1), hasInit(0, InitBase)),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 07a79d6bbe807..6d909f9d7c6bd 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -137,6 +137,10 @@ Changes in existing checks
<clang-tidy/checks/performance/move-const-arg>` check by fixing false negatives
on ternary operators calling ``std::move``.
+- Improved :doc:`modernize-use-default-member-init
+ <clang-tidy/checks/modernize/use-default-member-init>` check by matching arithmetic
+ operations within member list initialization.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp
index 81c980e0217e6..ff8c80b682bdb 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init.cpp
@@ -518,3 +518,29 @@ class ArrayBraceInitMultipleValues {
};
} // namespace PR63285
+
+namespace PR122480 {
+
+#define ARITHMETIC_MACRO (44 - 2)
+
+class DefaultMemberInitWithArithmetic {
+ DefaultMemberInitWithArithmetic() : a{1 + 1}, b{1 + 11 + 123 + 1234}, c{2 + (4 / 2) + 3 + (7 / 11)}, d{ARITHMETIC_MACRO * 2}, e{1.2 + 3.4} {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: member initializer for 'a' is redundant [modernize-use-default-member-init]
+ // CHECK-FIXES: DefaultMemberInitWithArithmetic() {}
+
+ int a{1 + 1};
+ int b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'b' [modernize-use-default-member-init]
+ // CHECK-FIXES: int b{1 + 11 + 123 + 1234};
+ int c;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'c' [modernize-use-default-member-init]
+ // CHECK-FIXES: int c{2 + (4 / 2) + 3 + (7 / 11)}
+ int d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'd' [modernize-use-default-member-init]
+ // CHECK-FIXES: int d{ARITHMETIC_MACRO * 2};
+ double e;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use default member initializer for 'e' [modernize-use-default-member-init]
+ // CHECK-FIXES: double e{1.2 + 3.4};
+
+};
+} // namespace PR122480
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…zation in modernize-use-default-member-init
60856b9
to
68c3719
Compare
@@ -202,7 +208,13 @@ void UseDefaultMemberInitCheck::registerMatchers(MatchFinder *Finder) { | |||
unaryOperator(hasAnyOperatorName("+", "-"), | |||
hasUnaryOperand(floatLiteral())), | |||
cxxBoolLiteral(), cxxNullPtrLiteralExpr(), implicitValueInitExpr(), | |||
declRefExpr(to(enumConstantDecl()))); | |||
declRefExpr(to(enumConstantDecl())), | |||
binaryOperator(hasLHS(anyOf(integerLiteral(), floatLiteral(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving duplicate code to helper-matcher like auto Init
@@ -137,6 +137,10 @@ Changes in existing checks | |||
<clang-tidy/checks/performance/move-const-arg>` check by fixing false negatives | |||
on ternary operators calling ``std::move``. | |||
|
|||
- Improved :doc:`modernize-use-default-member-init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep alphabetical order (by check name) in this list.
This aims to address a portion of #122480 by adding matchers on binary operators. This allows the detection of explicit arithmetic operations within initializers.
Note
I'm aware of the other false-negatives presented in the issue above, specifically not matching explicit castings & constexpr values. These will be addressed separately on a different PR's which are in the process and shouldn't take long (let me know if that's the right approach).
Feedback is much appreciated.