Improve warning messages (and readability) of connectSlotsByName()

While adding a test case for the new behavior, two issues
with the connectSlotsByName implementation came up:

  1. for auto-connected slots that don't exactly match a signal,
     a 'compatible' one is searched. There might be more than
     one of those. The implementation randomly picks any.
  2. The "No matching signal for %s" warning gets printed even for
     slots that can never be connected via connectSlotsMyName
     anyway (e.g. "on_something"). This is inconsistent.

This fixed both: an explicit warning is printed if more than one
'compatible' signal is found and the "No matching signal for %s"
warning is only printed if the slot adheres to the full
"on_child_signal()" naming convention.

In the process I added comments and changed the code slightly to
make it more readable and explicitly hint at non-obvious behavior.

Change-Id: Icc8e3b9936188d2da8dfff9f0373c8e5c776eb14
Reviewed-by: Olivier Goffart <ogoffart@woboq.com>
Reviewed-by: Friedemann Kleint <Friedemann.Kleint@digia.com>

2 files changed, 45 insertions, 12 deletions

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 617fadc1ab..3e578b36d7 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -3234,38 +3234,62 @@ void QMetaObject::connectSlotsByName(QObject *o)
             o->findChildren<QObject *>(QString()) // all children of 'o'...
             << o; // and the object 'o' itself
 
+    // for each method/slot of o ...
     for (int i = 0; i < mo->methodCount(); ++i) {
-        QByteArray slotSignature = mo->method(i).methodSignature();
+        const QByteArray slotSignature = mo->method(i).methodSignature();
         const char *slot = slotSignature.constData();
         Q_ASSERT(slot);
+
+        // ...that starts with "on_", ...
         if (slot[0] != 'o' || slot[1] != 'n' || slot[2] != '_')
             continue;
+
+        // ...we check each object in our list, ...
         bool foundIt = false;
         for(int j = 0; j < list.count(); ++j) {
             const QObject *co = list.at(j);
-            QByteArray objName = co->objectName().toLatin1();
-            int len = objName.length();
-            if (!len || qstrncmp(slot + 3, objName.data(), len) || slot[len+3] != '_')
+            const QByteArray coName = co->objectName().toLatin1();
+
+            // ...discarding those whose objectName is not fitting the pattern "on_<objectName>_...", ...
+            if (coName.isEmpty() || qstrncmp(slot + 3, coName.constData(), coName.size()) || slot[coName.size()+3] != '_')
                 continue;
+
+            const char *signal = slot + coName.size() + 4; // the 'signal' part of the slot name
+
+            // ...for the presence of a matching signal "on_<objectName>_<signal>".
             const QMetaObject *smeta;
-            int sigIndex = co->d_func()->signalIndex(slot + len + 4, &smeta);
-            if (sigIndex < 0) { // search for compatible signals
+            int sigIndex = co->d_func()->signalIndex(signal, &smeta);
+            if (sigIndex < 0) {
+                // if no exactly fitting signal (name + complete parameter type list) could be found
+                // look for just any signal with the correct name and at least the slot's parameter list.
+                // Note: if more than one of thoses signals exist, the one that gets connected is
+                // chosen 'at random' (order of declaration in source file)
+                QList<QByteArray> compatibleSignals;
                 const QMetaObject *smo = co->metaObject();
-                int slotlen = qstrlen(slot + len + 4) - 1;
-                for (int k = 0; k < QMetaObjectPrivate::absoluteSignalCount(smo); ++k) {
-                    QMetaMethod method = QMetaObjectPrivate::signal(smo, k);
-                    if (!qstrncmp(method.methodSignature().constData(), slot + len + 4, slotlen)) {
+                int sigLen = qstrlen(signal) - 1; // ignore the trailing ')'
+                for (int k = QMetaObjectPrivate::absoluteSignalCount(smo)-1; k >= 0; --k) {
+                    const QMetaMethod method = QMetaObjectPrivate::signal(smo, k);
+                    if (!qstrncmp(method.methodSignature().constData(), signal, sigLen)) {
                         smeta = method.enclosingMetaObject();
                         sigIndex = k;
-                        break;
+                        compatibleSignals.prepend(method.methodSignature());
                     }
                 }
+                if (compatibleSignals.size() > 1)
+                    qWarning() << "QMetaObject::connectSlotsByName: Connecting slot" << slot
+                               << "with the first of the following compatible signals:" << compatibleSignals;
             }
+
             if (sigIndex < 0)
                 continue;
 
+            // we connect it...
             if (Connection(QMetaObjectPrivate::connect(co, sigIndex, smeta, o, i))) {
                 foundIt = true;
+                // ...and stop looking for further objects with the same name.
+                // Note: the Designer will make sure each object name is unique in the above
+                // 'list' but other code may create two child objects with the same name. In
+                // this case one is chosen 'at random'.
                 break;
             }
         }
@@ -3274,7 +3298,11 @@ void QMetaObject::connectSlotsByName(QObject *o)
             while (mo->method(i + 1).attributes() & QMetaMethod::Cloned)
                   ++i;
         } else if (!(mo->method(i).attributes() & QMetaMethod::Cloned)) {
-            qWarning("QMetaObject::connectSlotsByName: No matching signal for %s", slot);
+            // check if the slot has the following signature: "on_..._...(..."
+            int iParen = slotSignature.indexOf('(');
+            int iLastUnderscore = slotSignature.lastIndexOf('_', iParen-1);
+            if (iLastUnderscore > 3)
+                qWarning("QMetaObject::connectSlotsByName: No matching signal for %s", slot);
         }
     }
 }
diff --git a/tests/auto/corelib/kernel/qobject/tst_qobject.cpp b/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
index db21ab4fe4..dd88d863c9 100644
--- a/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
+++ b/tests/auto/corelib/kernel/qobject/tst_qobject.cpp
@@ -427,6 +427,9 @@ public slots:
 protected slots:
     void o() { called_slots << -1; }
     void on() { called_slots << -1; }
+    void on_() { called_slots << -1; }
+    void on_something() { called_slots << -1; }
+    void on_child_signal() { called_slots << -1; }
 
 signals:
     void on_Sender_signalLoopBack();
@@ -441,6 +444,8 @@ void tst_QObject::connectSlotsByName()
     AutoConnectSender sender(&receiver);
     sender.setObjectName("Sender");
 
+    QTest::ignoreMessage(QtWarningMsg, "QMetaObject::connectSlotsByName: No matching signal for on_child_signal()");
+    QTest::ignoreMessage(QtWarningMsg, "QMetaObject::connectSlotsByName: Connecting slot on_Sender_signalManyParams() with the first of the following compatible signals: (\"signalManyParams(int,int,int,QString,bool)\", \"signalManyParams(int,int,int,QString,bool,bool)\") ");
     QMetaObject::connectSlotsByName(&receiver);
 
     receiver.called_slots.clear();