From d2f663eebc30984e3ac8b4eb5adb54efd2d61b6b Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Fri, 11 Aug 2023 10:45:51 +0200 Subject: [PATCH] moc/QMetaProperty: Remove limitation on non-own-class notify signals The moc generated code does a sanity check that NOTIFY signals actually exist in the parent class when they cannot be found in the class moc currently runs on. The logic there was however too simplistic, and couldn't deal with signals taking a parameter. Fix this, and take the opportunity to use a proper static_assert instead of generating a "normal" compile error. We do not do any checks for the presence of QPrivateSignal, as the whole point of QPrivateSignal is that it should be private (and not e.g. protected). Moreover, we adjust QMetaProperty::notifySignalIndex to take single-argument notify methods into consideration as well when encontering an unresolved notify index. Fixes: QTBUG-115989 Change-Id: I8a056a15777f3132691e207b4b9ab6c2c9b2126d Reviewed-by: Ivan Solovev Reviewed-by: Volker Hilsheimer Reviewed-by: Fabian Kosmale (cherry picked from commit ac001bef798b79f4932d7ca8f4fb812159ba75ca) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/kernel/qmetaobject.cpp | 31 ++++++++++--------- src/tools/moc/generator.cpp | 29 +++++++++++++---- src/tools/moc/moc.cpp | 1 + .../qmetaproperty/tst_qmetaproperty.cpp | 28 ++++++++++++++++- tests/auto/tools/moc/tst_moc.cpp | 18 +++++++++++ 5 files changed, 86 insertions(+), 21 deletions(-) diff --git a/src/corelib/kernel/qmetaobject.cpp b/src/corelib/kernel/qmetaobject.cpp index 3d0eeb1683d..d45673ef25b 100644 --- a/src/corelib/kernel/qmetaobject.cpp +++ b/src/corelib/kernel/qmetaobject.cpp @@ -3973,20 +3973,23 @@ int QMetaProperty::notifySignalIndex() const if (!mobj || data.notifyIndex() == std::numeric_limits::max()) return -1; uint methodIndex = data.notifyIndex(); - if (methodIndex & IsUnresolvedSignal) { - methodIndex &= ~IsUnresolvedSignal; - const QByteArray signalName = stringData(mobj, methodIndex); - const QMetaObject *m = mobj; - const int idx = QMetaObjectPrivate::indexOfMethodRelative(&m, signalName, 0, nullptr); - if (idx >= 0) { - return idx + m->methodOffset(); - } else { - qWarning("QMetaProperty::notifySignal: cannot find the NOTIFY signal %s in class %s for property '%s'", - signalName.constData(), mobj->className(), name()); - return -1; - } - } - return methodIndex + mobj->methodOffset(); + if (!(methodIndex & IsUnresolvedSignal)) + return methodIndex + mobj->methodOffset(); + methodIndex &= ~IsUnresolvedSignal; + const QByteArray signalName = stringData(mobj, methodIndex); + const QMetaObject *m = mobj; + // try 0-arg signal + int idx = QMetaObjectPrivate::indexOfMethodRelative(&m, signalName, 0, nullptr); + if (idx >= 0) + return idx + m->methodOffset(); + // try 1-arg signal + QArgumentType argType(typeId()); + idx = QMetaObjectPrivate::indexOfMethodRelative(&m, signalName, 1, &argType); + if (idx >= 0) + return idx + m->methodOffset(); + qWarning("QMetaProperty::notifySignal: cannot find the NOTIFY signal %s in class %s for property '%s'", + signalName.constData(), mobj->className(), name()); + return -1; } // This method has been around for a while, but the documentation was marked \internal until 5.1 diff --git a/src/tools/moc/generator.cpp b/src/tools/moc/generator.cpp index 967d7236de1..02e9ef178ab 100644 --- a/src/tools/moc/generator.cpp +++ b/src/tools/moc/generator.cpp @@ -637,12 +637,29 @@ void Generator::generateCode() // Generate function to make sure the non-class signals exist in the parent classes // if (!cdef->nonClassSignalList.isEmpty()) { - fprintf(out, "// If you get a compile error in this function it can be because either\n"); - fprintf(out, "// a) You are using a NOTIFY signal that does not exist. Fix it.\n"); - fprintf(out, "// b) You are using a NOTIFY signal that does exist (in a parent class) but has a non-empty parameter list. This is a moc limitation.\n"); - fprintf(out, "[[maybe_unused]] static void checkNotifySignalValidity_%s(%s *t) {\n", qualifiedClassNameIdentifier.constData(), cdef->qualified.constData()); - for (const QByteArray &nonClassSignal : std::as_const(cdef->nonClassSignalList)) - fprintf(out, " t->%s();\n", nonClassSignal.constData()); + fprintf(out, "namespace CheckNotifySignalValidity_%s {\n", qualifiedClassNameIdentifier.constData()); + for (const QByteArray &nonClassSignal : std::as_const(cdef->nonClassSignalList)) { + const auto propertyIt = std::find_if(cdef->propertyList.constBegin(), + cdef->propertyList.constEnd(), + [&nonClassSignal](const PropertyDef &p) { + return nonClassSignal == p.notify; + }); + // must find something, otherwise checkProperties wouldn't have inserted an entry into nonClassSignalList + Q_ASSERT(propertyIt != cdef->propertyList.constEnd()); + fprintf(out, "template using has_nullary_%s = decltype(std::declval().%s());\n", + nonClassSignal.constData(), + nonClassSignal.constData()); + const auto &propertyType = propertyIt->type; + fprintf(out, "template using has_unary_%s = decltype(std::declval().%s(std::declval<%s>()));\n", + nonClassSignal.constData(), + nonClassSignal.constData(), + propertyType.constData()); + fprintf(out, "static_assert(qxp::is_detected_v || qxp::is_detected_v,\n" + " \"NOTIFY signal %s does not exist in class (or is private in its parent)\");\n", + nonClassSignal.constData(), cdef->qualified.constData(), + nonClassSignal.constData(), cdef->qualified.constData(), + nonClassSignal.constData()); + } fprintf(out, "}\n"); } } diff --git a/src/tools/moc/moc.cpp b/src/tools/moc/moc.cpp index e100ff4ba83..b471418a274 100644 --- a/src/tools/moc/moc.cpp +++ b/src/tools/moc/moc.cpp @@ -1167,6 +1167,7 @@ void Moc::generate(FILE *out, FILE *jsonOutput) fprintf(out, "\n#include \n"); fprintf(out, "\n#include \n\n"); // For std::addressof + fprintf(out, "\n#include \n"); // is_detected fprintf(out, "#if !defined(Q_MOC_OUTPUT_REVISION)\n" "#error \"The header file '%s' doesn't include .\"\n", fn.constData()); diff --git a/tests/auto/corelib/kernel/qmetaproperty/tst_qmetaproperty.cpp b/tests/auto/corelib/kernel/qmetaproperty/tst_qmetaproperty.cpp index f459068bef1..64f946f4309 100644 --- a/tests/auto/corelib/kernel/qmetaproperty/tst_qmetaproperty.cpp +++ b/tests/auto/corelib/kernel/qmetaproperty/tst_qmetaproperty.cpp @@ -22,7 +22,14 @@ struct CustomType Q_DECLARE_METATYPE(CustomType) -class tst_QMetaProperty : public QObject +class Base : public QObject +{ + Q_OBJECT +signals: + void baseSignal(int); +}; + +class tst_QMetaProperty : public Base { Q_OBJECT Q_PROPERTY(EnumType value WRITE setValue READ getValue) @@ -33,6 +40,7 @@ class tst_QMetaProperty : public QObject Q_PROPERTY(int value10 READ value10 FINAL) Q_PROPERTY(QMap map MEMBER map) Q_PROPERTY(CustomType custom MEMBER custom) + Q_PROPERTY(int propWithInheritedSig READ propWithInheritedSig NOTIFY baseSignal) private slots: void hasStdCppSet(); @@ -43,6 +51,7 @@ private slots: void mapProperty(); void conversion(); void enumsFlags(); + void notifySignalIndex(); public: enum EnumType { EnumType1 }; @@ -57,6 +66,8 @@ public: int value9() const { return 1; } int value10() const { return 1; } + int propWithInheritedSig() const { return 0; } + QString value7; QMap map; CustomType custom; @@ -274,5 +285,20 @@ void tst_QMetaProperty::enumsFlags() QCOMPARE(t.flagProperty(), EnumFlagsTester::flag2); } + +void tst_QMetaProperty::notifySignalIndex() +{ + auto mo = this->metaObject(); + auto propIndex = mo->indexOfProperty("propWithInheritedSig"); + auto propWithInheritedSig = mo->property(propIndex); + QVERIFY(propWithInheritedSig.isValid()); + QVERIFY(propWithInheritedSig.hasNotifySignal()); + QVERIFY(propWithInheritedSig.notifySignalIndex() != -1); + QMetaMethod notifySignal = propWithInheritedSig.notifySignal(); + QVERIFY(notifySignal.isValid()); + QCOMPARE(notifySignal.name(), "baseSignal"); + QCOMPARE(notifySignal.parameterCount(), 1); +} + QTEST_MAIN(tst_QMetaProperty) #include "tst_qmetaproperty.moc" diff --git a/tests/auto/tools/moc/tst_moc.cpp b/tests/auto/tools/moc/tst_moc.cpp index 3a72e9bf4b2..1726d096781 100644 --- a/tests/auto/tools/moc/tst_moc.cpp +++ b/tests/auto/tools/moc/tst_moc.cpp @@ -257,6 +257,24 @@ public: CreatableGadget creatableGadget; // Force the compiler to use the constructor +struct ParentWithSignalWithArgument : QObject { + Q_OBJECT + Q_PROPERTY(int i READ i WRITE setI NOTIFY iChanged) + +public: + int i() const {return 0;} + void setI(int) {} + +signals: + void iChanged(int); +}; + +struct SignalWithArgumentInParent : ParentWithSignalWithArgument +{ + Q_OBJECT + Q_PROPERTY(int otherI READ i WRITE setI NOTIFY iChanged) +}; + struct MyStruct {}; struct MyStruct2 {};