-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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][performance-move-const-arg] Fix crash when argument type has no definition #111472
Conversation
@llvm/pr-subscribers-clang-tidy Author: Nicolas van Kempen (nicovank) ChangesFix #111450. When the argument type is forward-declared and there is no definition,
Check Full diff: https://github.com/llvm/llvm-project/pull/111472.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
index d29b9e91f2e35d..421ce003975bc9 100644
--- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
@@ -209,8 +209,9 @@ void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) {
}
if (const CXXRecordDecl *RecordDecl = ArgType->getAsCXXRecordDecl();
- RecordDecl && !(RecordDecl->hasMoveConstructor() &&
- RecordDecl->hasMoveAssignment())) {
+ RecordDecl && RecordDecl->hasDefinition() &&
+ !(RecordDecl->hasMoveConstructor() &&
+ RecordDecl->hasMoveAssignment())) {
const bool MissingMoveAssignment = !RecordDecl->hasMoveAssignment();
const bool MissingMoveConstructor = !RecordDecl->hasMoveConstructor();
const bool MissingBoth = MissingMoveAssignment && MissingMoveConstructor;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp
index 4505eef6df24bd..8e325b0ae6ca30 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp
@@ -546,3 +546,17 @@ void testAlsoNonMoveable() {
}
} // namespace issue_62550
+
+namespace GH111450 {
+struct Status;
+
+struct Error {
+ Error(const Status& S);
+};
+
+struct Result {
+ Error E;
+ Result(Status&& S) : E(std::move(S)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg]
+};
+} // namespace GH111450
|
@llvm/pr-subscribers-clang-tools-extra Author: Nicolas van Kempen (nicovank) ChangesFix #111450. When the argument type is forward-declared and there is no definition,
Check Full diff: https://github.com/llvm/llvm-project/pull/111472.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
index d29b9e91f2e35d..421ce003975bc9 100644
--- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
@@ -209,8 +209,9 @@ void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) {
}
if (const CXXRecordDecl *RecordDecl = ArgType->getAsCXXRecordDecl();
- RecordDecl && !(RecordDecl->hasMoveConstructor() &&
- RecordDecl->hasMoveAssignment())) {
+ RecordDecl && RecordDecl->hasDefinition() &&
+ !(RecordDecl->hasMoveConstructor() &&
+ RecordDecl->hasMoveAssignment())) {
const bool MissingMoveAssignment = !RecordDecl->hasMoveAssignment();
const bool MissingMoveConstructor = !RecordDecl->hasMoveConstructor();
const bool MissingBoth = MissingMoveAssignment && MissingMoveConstructor;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp
index 4505eef6df24bd..8e325b0ae6ca30 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp
@@ -546,3 +546,17 @@ void testAlsoNonMoveable() {
}
} // namespace issue_62550
+
+namespace GH111450 {
+struct Status;
+
+struct Error {
+ Error(const Status& S);
+};
+
+struct Result {
+ Error E;
+ Result(Status&& S) : E(std::move(S)) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:{{[0-9]+}}: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg]
+};
+} // namespace GH111450
|
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 add a release note, otherwise LGTM
… has no definition Fix llvm#111450. When the argument type is forward-declared and there is no definition, `hasMoveConstructor` triggers the assert here: https://github.com/llvm/llvm-project/blob/7f65377880ce6a0e5eaa4cb2591b86b8c8a24ee6/clang/include/clang/AST/DeclCXX.h#L465 https://github.com/llvm/llvm-project/blob/7f65377880ce6a0e5eaa4cb2591b86b8c8a24ee6/clang/include/clang/AST/DeclCXX.h#L864 Check `hasDefinition()` before `hasMoveConstructor()`.
Fix #111450.
When the argument type is forward-declared and there is no definition,
hasMoveConstructor
triggers the assert here:llvm-project/clang/include/clang/AST/DeclCXX.h
Line 465 in 7f65377
llvm-project/clang/include/clang/AST/DeclCXX.h
Line 864 in 7f65377
Check
hasDefinition()
beforehasMoveConstructor()
.