[sdk/clazy] /: old-style-connect: Do not add .data to QPointer connect parameters
Alexander Lohnau <[email protected]>
| Newsgroups | gmane.comp.kde.cvs |
|---|---|
| Message-ID | <[email protected]> |
Git commit c4fc43ea5443a0281b1dc01277b6165b6f5df8b4 by Alexander Lohnau.
Committed on 02/08/2026 at 18:33.
Pushed by alex into branch 'master'.
old-style-connect: Do not add .data to QPointer connect parameters
This basically reverts
https://invent.kde.org/sdk/clazy/-/commit/1558e7b0ca176567151e78590e8cf8a8d3606983
because the gcc version is ancient and modern versions do not have this issue.
Also, simplify the control flow a bit to have like a "remember
lastRecordDecl and contiue" section rathern than a huge nested block
M +1 -0 Changelog
M +107 -117 src/checks/level2/old-style-connect.cpp
M +2 -2 tests/old-style-connect/main.cpp
M +0 -1 tests/old-style-connect/main.cpp.expected
M +0 -1 tests/old-style-connect/main.cpp.expected.tidy
M +7 -7 tests/old-style-connect/main.cpp.fixed.expected
https://invent.kde.org/sdk/clazy/-/commit/c4fc43ea5443a0281b1dc01277b6165b6f5df8b4
diff --git a/Changelog b/Changelog
index 6f63fa53..fa3141b4 100644
--- a/Changelog
+++ b/Changelog
@@ -6,6 +6,7 @@ Bugfixes:
* fully-qualified-moc-types no longer provides false positive for const pointers
* detaching-temporary/detaching-member fixits no longer provide false source ranges when used within macros
* compare-member-check: Fix clazy crashing when methods have no body, BUG: 518098
+ * old-style-connect: Do not add ".data()" to QObject connect parameters. This was needed in the past due to an old GCC bug.
Features:
* New check modernize-overloaded-connects: Finds QOverload/qOverload/static_cast statements for connects that are no longer needed and suggest removing them.
In case they are still needed, a fixit using qOverload is provided. QOverload/static_cast statements will have a fixit to modernize.
diff --git a/src/checks/level2/old-style-connect.cpp b/src/checks/level2/old-style-connect.cpp
index 7461a27f..6b93c6fe 100644
--- a/src/checks/level2/old-style-connect.cpp
+++ b/src/checks/level2/old-style-connect.cpp
@@ -334,154 +334,144 @@ std::vector<FixItHint> OldStyleConnect::fixits(int classification, T *callOrCtor
for (auto arg : callOrCtor->arguments()) {
SourceLocation s = arg->getBeginLoc();
static const CXXRecordDecl *lastRecordDecl = nullptr;
- if (isSignalOrSlot(s, macroName)) {
- macroNum++;
- if (!lastRecordDecl && (classification & ConnectFlag_4ArgsConnect)) {
- // This means it's a connect with implicit receiver
- lastRecordDecl = Utils::recordForMemberCall(dyn_cast<CXXMemberCallExpr>(callOrCtor), implicitCallee);
-
- if (macroNum == 1) {
- llvm::errs() << "This first macro shouldn't enter this path";
- }
- if (!lastRecordDecl) {
- std::string msg = "Failed to get class name for implicit receiver";
- queueManualFixitWarning(s, msg);
- return {};
- }
+ if (!isSignalOrSlot(s, macroName)) {
+ if (auto *record = clazy::getBestDynamicClassType(arg)) {
+ lastRecordDecl = record;
}
+ continue;
+ }
+ macroNum++;
+ if (!lastRecordDecl && (classification & ConnectFlag_4ArgsConnect)) {
+ // This means it's a connect with implicit receiver
+ lastRecordDecl = Utils::recordForMemberCall(dyn_cast<CXXMemberCallExpr>(callOrCtor), implicitCallee);
- if (!lastRecordDecl || !lastRecordDecl->hasDefinition()) {
- std::string msg = "Failed to get class name for explicit receiver";
+ if (macroNum == 1) {
+ llvm::errs() << "This first macro shouldn't enter this path";
+ }
+ if (!lastRecordDecl) {
+ std::string msg = "Failed to get class name for implicit receiver";
queueManualFixitWarning(s, msg);
return {};
}
+ }
- const std::string methodName = signalOrSlotNameFromMacro(s);
-
- // With LLVM22, we need to explicitly ask for the definition
- auto methods = Utils::methodsFromString(lastRecordDecl->getDefinition(), methodName);
- if (methods.empty()) {
- std::string msg;
- if (isPrivateSlot(methodName)) {
- msg = "Converting Q_PRIVATE_SLOTS not implemented yet\n";
- } else {
- if (m_context->isQtDeveloper() && classIsOk(clazy::name(lastRecordDecl))) {
- // This is OK
- return {};
- }
- msg = "No such method " + methodName + " in class " + lastRecordDecl->getNameAsString();
- }
+ if (!lastRecordDecl || !lastRecordDecl->hasDefinition()) {
+ std::string msg = "Failed to get class name for explicit receiver";
+ queueManualFixitWarning(s, msg);
+ return {};
+ }
- queueManualFixitWarning(s, msg);
- return {};
- }
- if (methods.size() != 1) {
- std::string msg = std::string("Too many overloads (") + std::to_string(methods.size()) + std::string(") for method ") + methodName
- + " for record " + lastRecordDecl->getNameAsString();
- queueManualFixitWarning(s, msg);
- return {};
+ const std::string methodName = signalOrSlotNameFromMacro(s);
+
+ // With LLVM22, we need to explicitly ask for the definition
+ auto methods = Utils::methodsFromString(lastRecordDecl->getDefinition(), methodName);
+ if (methods.empty()) {
+ std::string msg;
+ if (isPrivateSlot(methodName)) {
+ msg = "Converting Q_PRIVATE_SLOTS not implemented yet\n";
} else {
- AccessSpecifierManager *a = m_context->accessSpecifierManager;
- if (!a) {
- return {};
- }
- const bool isSignal = a->qtAccessSpecifierType(methods[0]) == QtAccessSpecifier_Signal;
- if (isSignal && macroName == "SLOT") {
- // The method is actually a signal and the user used SLOT()
- // bail out with the fixing.
- std::string msg = std::string("Can't fix. SLOT macro used but method " + methodName + " is a signal");
- queueManualFixitWarning(s, msg);
+ if (m_context->isQtDeveloper() && classIsOk(clazy::name(lastRecordDecl))) {
+ // This is OK
return {};
}
+ msg = "No such method " + methodName + " in class " + lastRecordDecl->getNameAsString();
}
- auto *methodDecl = methods[0];
- if (methodDecl->isStatic()) {
+ queueManualFixitWarning(s, msg);
+ return {};
+ }
+ if (methods.size() != 1) {
+ std::string msg = std::string("Too many overloads (") + std::to_string(methods.size()) + std::string(") for method ") + methodName + " for record "
+ + lastRecordDecl->getNameAsString();
+ queueManualFixitWarning(s, msg);
+ return {};
+ } else {
+ AccessSpecifierManager *a = m_context->accessSpecifierManager;
+ if (!a) {
return {};
}
-
- if (macroNum == 1) {
- // Save the number of parameters of the signal. The slot should not have more arguments.
- senderMethod = methodDecl;
- } else if (macroNum == 2) {
- const unsigned int numReceiverParams = methodDecl->getNumParams();
- if (numReceiverParams > senderMethod->getNumParams()) {
- std::string msg = std::string("Receiver has more parameters (") + std::to_string(methodDecl->getNumParams()) + ") than signal ("
- + std::to_string(senderMethod->getNumParams()) + ')';
- queueManualFixitWarning(s, msg);
- return {};
- }
-
- for (unsigned int i = 0; i < numReceiverParams; ++i) {
- ParmVarDecl *receiverParm = methodDecl->getParamDecl(i);
- ParmVarDecl *senderParm = senderMethod->getParamDecl(i);
- if (!clazy::isConvertibleTo(senderParm->getType().getTypePtr(), receiverParm->getType().getTypePtrOrNull())) {
- std::string msg("Sender's parameters are incompatible with the receiver's");
- queueManualFixitWarning(s, msg);
- return {};
- }
- }
- }
-
- if ((classification & ConnectFlag_QTimerSingleShot) && methodDecl->getNumParams() > 0) {
- std::string msg = "(QTimer) Fixit not implemented for slot with arguments, use a lambda";
+ const bool isSignal = a->qtAccessSpecifierType(methods[0]) == QtAccessSpecifier_Signal;
+ if (isSignal && macroName == "SLOT") {
+ // The method is actually a signal and the user used SLOT()
+ // bail out with the fixing.
+ std::string msg = std::string("Can't fix. SLOT macro used but method " + methodName + " is a signal");
queueManualFixitWarning(s, msg);
return {};
}
+ }
- if ((classification & ConnectFlag_QMenuAddAction) && methodDecl->getNumParams() > 0) {
- std::string msg = "(QMenu) Fixit not implemented for slot with arguments, use a lambda";
+ auto *methodDecl = methods[0];
+ if (methodDecl->isStatic()) {
+ return {};
+ }
+
+ if (macroNum == 1) {
+ // Save the number of parameters of the signal. The slot should not have more arguments.
+ senderMethod = methodDecl;
+ } else if (macroNum == 2) {
+ const unsigned int numReceiverParams = methodDecl->getNumParams();
+ if (numReceiverParams > senderMethod->getNumParams()) {
+ std::string msg = std::string("Receiver has more parameters (") + std::to_string(methodDecl->getNumParams()) + ") than signal ("
+ + std::to_string(senderMethod->getNumParams()) + ')';
queueManualFixitWarning(s, msg);
return {};
}
- DeclContext *context = m_context->lastDecl->getDeclContext();
-
- bool isSpecialProtectedCase = false;
- if (!clazy::canTakeAddressOf(methodDecl, context, /*by-ref*/ isSpecialProtectedCase)) {
- std::string msg = "Can't fix " + clazy::accessString(methodDecl->getAccess()) + ' ' + macroName + ' ' + methodDecl->getQualifiedNameAsString();
- queueManualFixitWarning(s, msg);
- return {};
+ for (unsigned int i = 0; i < numReceiverParams; ++i) {
+ ParmVarDecl *receiverParm = methodDecl->getParamDecl(i);
+ ParmVarDecl *senderParm = senderMethod->getParamDecl(i);
+ if (!clazy::isConvertibleTo(senderParm->getType().getTypePtr(), receiverParm->getType().getTypePtrOrNull())) {
+ std::string msg("Sender's parameters are incompatible with the receiver's");
+ queueManualFixitWarning(s, msg);
+ return {};
+ }
}
+ }
- std::string qualifiedName;
- auto *contextRecord = clazy::firstContextOfType<CXXRecordDecl>(m_context->lastDecl->getDeclContext());
- const bool isInInclude = sm().getMainFileID() != sm().getFileID(locStart);
+ if ((classification & ConnectFlag_QTimerSingleShot) && methodDecl->getNumParams() > 0) {
+ std::string msg = "(QTimer) Fixit not implemented for slot with arguments, use a lambda";
+ queueManualFixitWarning(s, msg);
+ return {};
+ }
- if (isSpecialProtectedCase && contextRecord) {
- // We're inside a derived class trying to take address of a protected base member, must use &Derived::method instead of &Base::method.
- qualifiedName = contextRecord->getNameAsString() + "::" + methodDecl->getNameAsString();
- } else {
- qualifiedName = clazy::getMostNeededQualifiedName(sm(), methodDecl, context, locStart, !isInInclude); // (In includes ignore using directives)
- }
+ if ((classification & ConnectFlag_QMenuAddAction) && methodDecl->getNumParams() > 0) {
+ std::string msg = "(QMenu) Fixit not implemented for slot with arguments, use a lambda";
+ queueManualFixitWarning(s, msg);
+ return {};
+ }
- CharSourceRange expansionRange = sm().getImmediateExpansionRange(s);
- SourceRange range = SourceRange(expansionRange.getBegin(), expansionRange.getEnd());
+ DeclContext *context = m_context->lastDecl->getDeclContext();
- const std::string functionPointer = '&' + qualifiedName;
- std::string replacement = functionPointer;
+ bool isSpecialProtectedCase = false;
+ if (!clazy::canTakeAddressOf(methodDecl, context, /*by-ref*/ isSpecialProtectedCase)) {
+ std::string msg = "Can't fix " + clazy::accessString(methodDecl->getAccess()) + ' ' + macroName + ' ' + methodDecl->getQualifiedNameAsString();
+ queueManualFixitWarning(s, msg);
+ return {};
+ }
- if ((classification & ConnectFlag_4ArgsConnect) && macroNum == 2) {
- replacement = implicitCallee + ", " + replacement;
- }
+ std::string qualifiedName;
+ auto *contextRecord = clazy::firstContextOfType<CXXRecordDecl>(m_context->lastDecl->getDeclContext());
+ const bool isInInclude = sm().getMainFileID() != sm().getFileID(locStart);
- fixits.push_back(FixItHint::CreateReplacement(range, replacement));
- lastRecordDecl = nullptr;
+ if (isSpecialProtectedCase && contextRecord) {
+ // We're inside a derived class trying to take address of a protected base member, must use &Derived::method instead of &Base::method.
+ qualifiedName = contextRecord->getNameAsString() + "::" + methodDecl->getNameAsString();
} else {
- Expr *expr = arg;
- const auto *const record = clazy::getBestDynamicClassType(expr);
- if (record) {
- lastRecordDecl = record;
- if (isQPointer(expr)) {
- auto endLoc = clazy::locForNextToken(astContext(), arg->getBeginLoc(), tok::comma);
- if (endLoc.isValid()) {
- fixits.push_back(FixItHint::CreateInsertion(endLoc, ".data()"));
- } else {
- return {};
- }
- }
- }
+ qualifiedName = clazy::getMostNeededQualifiedName(sm(), methodDecl, context, locStart, !isInInclude); // (In includes ignore using directives)
+ }
+
+ CharSourceRange expansionRange = sm().getImmediateExpansionRange(s);
+ SourceRange range = SourceRange(expansionRange.getBegin(), expansionRange.getEnd());
+
+ const std::string functionPointer = '&' + qualifiedName;
+ std::string replacement = functionPointer;
+
+ if ((classification & ConnectFlag_4ArgsConnect) && macroNum == 2) {
+ replacement = implicitCallee + ", " + replacement;
}
+
+ fixits.push_back(FixItHint::CreateReplacement(range, replacement));
+ lastRecordDecl = nullptr;
}
return fixits;
diff --git a/tests/old-style-connect/main.cpp b/tests/old-style-connect/main.cpp
index a08a588d..a0c54cdd 100644
--- a/tests/old-style-connect/main.cpp
+++ b/tests/old-style-connect/main.cpp
@@ -190,7 +190,7 @@ class DerivedTestingProtected : public TestingProtected
void testQPointer()
{
QPointer<WithNesting> p8733 = new WithNesting();
- QObject::connect(p8733, SIGNAL(destroyed()), p8733, SLOT(deleteLater())); // Warning, and when fixed should have .data() due to gcc bug
+ QObject::connect(p8733, SIGNAL(destroyed()), p8733, SLOT(deleteLater())); // Warning
QObject::connect(p8733, &WithNesting::destroyed, p8733, &WithNesting::deleteLater);
QObject::connect(p8733.data(), SIGNAL(destroyed()), p8733.data(), SLOT(deleteLater())); // Warning
QObject::connect(p8733.data(), &WithNesting::destroyed, p8733.data(), &WithNesting::deleteLater);
@@ -319,7 +319,7 @@ public:
connect(ptr, SIGNAL(signal1()), SLOT(slot1()));
connect(ptr, SIGNAL(signal1()), ptr, SLOT(slot1()));
connect(p->ptr, SIGNAL(signal1()), p->ptr.data(), SLOT(slot1()));
- connect(d_func()->ptr, SIGNAL(signal1()), d_func()->ptr.data(), SLOT(slot1()));
+
ptr->disconnect(this);
}
diff --git a/tests/old-style-connect/main.cpp.expected b/tests/old-style-connect/main.cpp.expected
index 91f316e5..5186b268 100644
--- a/tests/old-style-connect/main.cpp.expected
+++ b/tests/old-style-connect/main.cpp.expected
@@ -69,7 +69,6 @@ old-style-connect/main.cpp:318:9: warning: Old Style Connect [-Wclazy-old-style-
old-style-connect/main.cpp:319:9: warning: Old Style Connect [-Wclazy-old-style-connect]
old-style-connect/main.cpp:320:9: warning: Old Style Connect [-Wclazy-old-style-connect]
old-style-connect/main.cpp:321:9: warning: Old Style Connect [-Wclazy-old-style-connect]
-old-style-connect/main.cpp:322:9: warning: Old Style Connect [-Wclazy-old-style-connect]
old-style-connect/main.cpp:341:5: warning: Old Style Connect [-Wclazy-old-style-connect]
old-style-connect/main.cpp:341:43: warning: FixIt failed, requires manual intervention: No such method foo in class QDBusInterface [-Wclazy-old-style-connect]
old-style-connect/main.cpp:365:5: warning: Old Style Connect [-Wclazy-old-style-connect]
diff --git a/tests/old-style-connect/main.cpp.expected.tidy b/tests/old-style-connect/main.cpp.expected.tidy
index c4fe48e3..da6ff7ff 100644
--- a/tests/old-style-connect/main.cpp.expected.tidy
+++ b/tests/old-style-connect/main.cpp.expected.tidy
@@ -69,7 +69,6 @@ old-style-connect/main.cpp:318:9: warning: Old Style Connect [-Wclazy-old-style-
old-style-connect/main.cpp:319:9: warning: Old Style Connect [-Wclazy-old-style-connect]
old-style-connect/main.cpp:320:9: warning: Old Style Connect [-Wclazy-old-style-connect]
old-style-connect/main.cpp:321:9: warning: Old Style Connect [-Wclazy-old-style-connect]
-old-style-connect/main.cpp:322:9: warning: Old Style Connect [-Wclazy-old-style-connect]
old-style-connect/main.cpp:341:5: warning: Old Style Connect [-Wclazy-old-style-connect]
old-style-connect/main.cpp:341:43: warning: FixIt failed, requires manual intervention: No such method foo in class QDBusInterface [-Wclazy-old-style-connect]
old-style-connect/main.cpp:365:5: warning: Old Style Connect [-Wclazy-old-style-connect]
diff --git a/tests/old-style-connect/main.cpp.fixed.expected b/tests/old-style-connect/main.cpp.fixed.expected
index bb29fde5..83c95f5d 100644
--- a/tests/old-style-connect/main.cpp.fixed.expected
+++ b/tests/old-style-connect/main.cpp.fixed.expected
@@ -190,7 +190,7 @@ class DerivedTestingProtected : public TestingProtected
void testQPointer()
{
QPointer<WithNesting> p8733 = new WithNesting();
- QObject::connect(p8733.data(), &QObject::destroyed, p8733.data(), &QObject::deleteLater); // Warning, and when fixed should have .data() due to gcc bug
+ QObject::connect(p8733, &QObject::destroyed, p8733, &QObject::deleteLater); // Warning
QObject::connect(p8733, &WithNesting::destroyed, p8733, &WithNesting::deleteLater);
QObject::connect(p8733.data(), &QObject::destroyed, p8733.data(), &QObject::deleteLater); // Warning
QObject::connect(p8733.data(), &WithNesting::destroyed, p8733.data(), &WithNesting::deleteLater);
@@ -314,12 +314,12 @@ public:
TestQPointerMember()
{
QPointer<MyObj> ptr;
- connect(m_ptr.data(), &MyObj::signal1, this, &TestQPointerMember::slot1);
- connect(m_ptr.data(), &MyObj::signal1, m_ptr.data(), &MyObj::slot1);
- connect(ptr.data(), &MyObj::signal1, this, &TestQPointerMember::slot1);
- connect(ptr.data(), &MyObj::signal1, ptr.data(), &MyObj::slot1);
- connect(p->ptr.data(), &MyObj::signal1, p->ptr.data(), &MyObj::slot1);
- connect(d_func()->ptr.data(), &MyObj::signal1, d_func()->ptr.data(), &MyObj::slot1);
+ connect(m_ptr, &MyObj::signal1, this, &TestQPointerMember::slot1);
+ connect(m_ptr, &MyObj::signal1, m_ptr, &MyObj::slot1);
+ connect(ptr, &MyObj::signal1, this, &TestQPointerMember::slot1);
+ connect(ptr, &MyObj::signal1, ptr, &MyObj::slot1);
+ connect(p->ptr, &MyObj::signal1, p->ptr.data(), &MyObj::slot1);
+
ptr->disconnect(this);
}