From 1986d8179d2edd4171bb1460e2a0ee97d6ee3e18 Mon Sep 17 00:00:00 2001 From: Friedemann Kleint Date: Mon, 15 Nov 2021 14:45:14 +0100 Subject: shiboken6: Generate rich comparison for smart pointers Add the pointee class to the GeneratorContext for smart pointers and generate a comparison operator for the pointee. Use the pointee's comparison operators if there are any; else generate a simple equality check based on pointee address. [ChangeLog][shiboken6] Comparison operators for pointees of smart pointers are now generated. Fixes: PYSIDE-1711 Change-Id: Ib21b90a4ccfe635ea051831a5b66a79ded06b194 Reviewed-by: Christian Tismer --- sources/shiboken6/generator/generator.cpp | 12 +- sources/shiboken6/generator/generator.h | 5 +- .../shiboken6/generator/shiboken/cppgenerator.cpp | 151 +++++++++++++++++++-- .../shiboken6/generator/shiboken/cppgenerator.h | 6 + sources/shiboken6/tests/libsmart/smart.cpp | 19 +++ sources/shiboken6/tests/libsmart/smart_integer.h | 32 +++++ sources/shiboken6/tests/libsmart/smart_obj.h | 5 +- .../tests/smartbinding/smart_pointer_test.py | 34 +++++ 8 files changed, 252 insertions(+), 12 deletions(-) diff --git a/sources/shiboken6/generator/generator.cpp b/sources/shiboken6/generator/generator.cpp index ef9cf3d47..49652a94d 100644 --- a/sources/shiboken6/generator/generator.cpp +++ b/sources/shiboken6/generator/generator.cpp @@ -465,12 +465,14 @@ GeneratorContext Generator::contextForClass(const AbstractMetaClass *c) const } GeneratorContext Generator::contextForSmartPointer(const AbstractMetaClass *c, - const AbstractMetaType &t) + const AbstractMetaType &t, + const AbstractMetaClass *pointeeClass) { GeneratorContext result; result.m_metaClass = c; result.m_preciseClassType = t; result.m_type = GeneratorContext::SmartPointer; + result.m_pointeeClass = pointeeClass; return result; } @@ -493,8 +495,14 @@ bool Generator::generate() smartPointers))); return false; } - if (!generateFileForContext(contextForSmartPointer(smartPointerClass, type))) + const AbstractMetaClass *pointeeClass = nullptr; + const auto *instantiatedType = type.instantiations().constFirst().typeEntry(); + if (instantiatedType->isComplex()) // not a C++ primitive + pointeeClass = AbstractMetaClass::findClass(m_d->api.classes(), instantiatedType); + if (!generateFileForContext(contextForSmartPointer(smartPointerClass, type, + pointeeClass))) { return false; + } } return finishGeneration(); } diff --git a/sources/shiboken6/generator/generator.h b/sources/shiboken6/generator/generator.h index 82f231b93..62c5e5aeb 100644 --- a/sources/shiboken6/generator/generator.h +++ b/sources/shiboken6/generator/generator.h @@ -154,6 +154,7 @@ public: const AbstractMetaClass *metaClass() const { return m_metaClass; } const AbstractMetaType &preciseType() const { return m_preciseClassType; } + const AbstractMetaClass *pointeeClass() const { return m_pointeeClass; } bool forSmartPointer() const { return m_type == SmartPointer; } bool useWrapper() const { return m_type == WrappedClass; } @@ -168,6 +169,7 @@ public: private: const AbstractMetaClass *m_metaClass = nullptr; + const AbstractMetaClass *m_pointeeClass = nullptr; AbstractMetaType m_preciseClassType; QString m_wrappername; Type m_type = Class; @@ -259,7 +261,8 @@ protected: virtual GeneratorContext contextForClass(const AbstractMetaClass *c) const; static GeneratorContext contextForSmartPointer(const AbstractMetaClass *c, - const AbstractMetaType &t); + const AbstractMetaType &t, + const AbstractMetaClass *pointeeClass = nullptr); /// Generates a file for given AbstractMetaClass or AbstractMetaType (smart pointer case). bool generateFileForContext(const GeneratorContext &context); diff --git a/sources/shiboken6/generator/shiboken/cppgenerator.cpp b/sources/shiboken6/generator/shiboken/cppgenerator.cpp index a9d41f1f0..61f2c8ac8 100644 --- a/sources/shiboken6/generator/shiboken/cppgenerator.cpp +++ b/sources/shiboken6/generator/shiboken/cppgenerator.cpp @@ -392,6 +392,12 @@ static void writePyGetSetDefEntry(TextStream &s, const QString &name, << (setFunc.isEmpty() ? QLatin1String(NULL_PTR) : setFunc) << "},\n"; } +static bool generateRichComparison(const GeneratorContext &c) +{ + return c.forSmartPointer() + || (!c.metaClass()->isNamespace() && c.metaClass()->hasComparisonOperatorOverload()); +} + /*! Function used to write the class generated binding code on the buffer \param s the output buffer @@ -737,9 +743,12 @@ void CppGenerator::generateClass(TextStream &s, const GeneratorContext &classCon writeMappingMethods(s, metaClass, classContext); } - if (!metaClass->isNamespace() && metaClass->hasComparisonOperatorOverload()) { + if (generateRichComparison(classContext)) { s << "// Rich comparison\n"; - writeRichCompareFunction(s, classContext); + if (classContext.forSmartPointer()) + writeSmartPointerRichCompareFunction(s, classContext); + else + writeRichCompareFunction(s, classContext); } if (shouldGenerateGetSetList(metaClass) && !classContext.forSmartPointer()) { @@ -4370,7 +4379,7 @@ void CppGenerator::writeClassDefinition(TextStream &s, tp_flags.append(QLatin1String("|Py_TPFLAGS_HAVE_GC")); QString tp_richcompare; - if (!metaClass->isNamespace() && metaClass->hasComparisonOperatorOverload()) + if (generateRichComparison(classContext)) tp_richcompare = cpythonBaseName(metaClass) + QLatin1String("_richcompare"); QString tp_getset; @@ -4904,11 +4913,10 @@ void CppGenerator::writeSetterFunction(TextStream &s, const QPropertySpec &prope << "return 0;\n" << outdent << "}\n\n"; } -void CppGenerator::writeRichCompareFunction(TextStream &s, - const GeneratorContext &context) const +void CppGenerator::writeRichCompareFunctionHeader(TextStream &s, + const QString &baseName, + const GeneratorContext &context) const { - const AbstractMetaClass *metaClass = context.metaClass(); - QString baseName = cpythonBaseName(metaClass); s << "static PyObject * "; s << baseName << "_richcompare(PyObject *self, PyObject *" << PYTHON_ARG << ", int op)\n{\n" << indent; @@ -4918,6 +4926,17 @@ void CppGenerator::writeRichCompareFunction(TextStream &s, << PYTHON_TO_CPPCONVERSION_STRUCT << ' ' << PYTHON_TO_CPP_VAR << ";\n"; writeUnusedVariableCast(s, QLatin1String(PYTHON_TO_CPP_VAR)); s << '\n'; +} + +static const char richCompareComment[] = + "// PYSIDE-74: By default, we redirect to object's tp_richcompare (which is `==`, `!=`).\n"; + +void CppGenerator::writeRichCompareFunction(TextStream &s, + const GeneratorContext &context) const +{ + const AbstractMetaClass *metaClass = context.metaClass(); + QString baseName = cpythonBaseName(metaClass); + writeRichCompareFunctionHeader(s, baseName, context); s << "switch (op) {\n"; { @@ -5012,13 +5031,19 @@ void CppGenerator::writeRichCompareFunction(TextStream &s, s << "default:\n"; { Indentation indent(s); - s << "// PYSIDE-74: By default, we redirect to object's tp_richcompare (which is `==`, `!=`).\n" + s << richCompareComment << "return FallbackRichCompare(self, " << PYTHON_ARG << ", op);\n" << "goto " << baseName << "_RichComparison_TypeError;\n"; } } s << "}\n\n"; + writeRichCompareFunctionFooter(s, baseName); +} + +void CppGenerator::writeRichCompareFunctionFooter(TextStream &s, + const QString &baseName) const +{ s << "if (" << PYTHON_RETURN_VAR << " && !PyErr_Occurred())\n"; { Indentation indent(s); @@ -5030,6 +5055,116 @@ void CppGenerator::writeRichCompareFunction(TextStream &s, << outdent << "}\n\n"; } +using ComparisonOperatorList = QList; + +// Return the available comparison operators for smart pointers +static ComparisonOperatorList smartPointeeComparisons(const GeneratorContext &context) +{ + Q_ASSERT(context.forSmartPointer()); + auto *te = context.preciseType().instantiations().constFirst().typeEntry(); + if (te->isExtendedCppPrimitive()) { // Primitive pointee types have all + return {AbstractMetaFunction::OperatorEqual, + AbstractMetaFunction::OperatorNotEqual, + AbstractMetaFunction::OperatorLess, + AbstractMetaFunction::OperatorLessEqual, + AbstractMetaFunction::OperatorGreater, + AbstractMetaFunction::OperatorGreaterEqual}; + } + + auto *pointeeClass = context.pointeeClass(); + if (!pointeeClass) + return {}; + + ComparisonOperatorList result; + const auto &comparisons = + pointeeClass->operatorOverloads(OperatorQueryOption::SymmetricalComparisonOp); + for (const auto &f : comparisons) { + const auto ct = f->comparisonOperatorType().value(); + if (!result.contains(ct)) + result.append(ct); + } + return result; +} + +void CppGenerator::writeSmartPointerRichCompareFunction(TextStream &s, + const GeneratorContext &context) const +{ + static const char selfPointeeVar[] = "cppSelfPointee"; + static const char cppArg0PointeeVar[] = "cppArg0Pointee"; + + const AbstractMetaClass *metaClass = context.metaClass(); + QString baseName = cpythonBaseName(metaClass); + writeRichCompareFunctionHeader(s, baseName, context); + + s << "if ("; + writeTypeCheck(s, context.preciseType(), QLatin1String(PYTHON_ARG)); + s << ") {\n" << indent; + writeArgumentConversion(s, context.preciseType(), QLatin1String(CPP_ARG0), + QLatin1String(PYTHON_ARG), metaClass); + + const auto *te = context.preciseType().typeEntry(); + Q_ASSERT(te->isSmartPointer()); + const auto *ste = static_cast(te); + + s << "const auto *" << selfPointeeVar << " = " << CPP_SELF_VAR + << '.' << ste->getter() << "();\n"; + s << "const auto *" << cppArg0PointeeVar << " = " << CPP_ARG0 + << '.' << ste->getter() << "();\n"; + + // If we have an object without any comparisons, only generate a simple + // equality check by pointee address + auto availableOps = smartPointeeComparisons(context); + const bool comparePointeeAddressOnly = availableOps.isEmpty(); + if (comparePointeeAddressOnly) { + availableOps << AbstractMetaFunction::OperatorEqual + << AbstractMetaFunction::OperatorNotEqual; + } else { + // For value types with operators, we complain about nullptr + s << "if (" << selfPointeeVar << " == nullptr || " << cppArg0PointeeVar + << " == nullptr) {\n" << indent + << "PyErr_SetString(PyExc_NotImplementedError, \"nullptr passed to comparison.\");\n" + << returnStatement(m_currentErrorCode) << '\n' << outdent << "}\n"; + } + + s << "bool " << CPP_RETURN_VAR << "= false;\n" + << "switch (op) {\n"; + for (auto op : availableOps) { + s << "case " << AbstractMetaFunction::pythonRichCompareOpCode(op) << ":\n" + << indent << CPP_RETURN_VAR << " = "; + if (comparePointeeAddressOnly) { + s << selfPointeeVar << ' ' << AbstractMetaFunction::cppComparisonOperator(op) + << ' ' << cppArg0PointeeVar << ";\n"; + } else { + // Shortcut for equality: Check pointee address + if (op == AbstractMetaFunction::OperatorEqual + || op == AbstractMetaFunction::OperatorLessEqual + || op == AbstractMetaFunction::OperatorGreaterEqual) { + s << selfPointeeVar << " == " << cppArg0PointeeVar << " || "; + } + // Generate object's comparison + s << "*" << selfPointeeVar << ' ' + << AbstractMetaFunction::cppComparisonOperator(op) << " *" + << cppArg0PointeeVar << ";\n"; + } + s << "break;\n" << outdent; + + } + if (availableOps.size() < 6) { + s << "default:\n" << indent + << richCompareComment + << "return FallbackRichCompare(self, " << PYTHON_ARG << ", op);\n" << outdent; + } + s << "}\n" << PYTHON_RETURN_VAR << " = " << CPP_RETURN_VAR + << " ? Py_True : Py_False;\n" + << "Py_INCREF(" << PYTHON_RETURN_VAR << ");\n"; + + s << outdent << "} else {\n" << indent + << "goto " << baseName << "_RichComparison_TypeError;\n" + << outdent << "}\n"; + + writeRichCompareFunctionFooter(s, baseName); +} + QString CppGenerator::methodDefinitionParameters(const OverloadData &overloadData) const { const bool usePyArgs = overloadData.pythonFunctionWrapperUsesListOfArguments(); diff --git a/sources/shiboken6/generator/shiboken/cppgenerator.h b/sources/shiboken6/generator/shiboken/cppgenerator.h index 3e3e5a21d..3b2428fb5 100644 --- a/sources/shiboken6/generator/shiboken/cppgenerator.h +++ b/sources/shiboken6/generator/shiboken/cppgenerator.h @@ -376,7 +376,13 @@ private: const QPropertySpec &property, const GeneratorContext &context) const; + void writeRichCompareFunctionHeader(TextStream &s, + const QString &baseName, + const GeneratorContext &context) const; + void writeRichCompareFunctionFooter(TextStream &s, + const QString &baseName) const; void writeRichCompareFunction(TextStream &s, const GeneratorContext &context) const; + void writeSmartPointerRichCompareFunction(TextStream &s, const GeneratorContext &context) const; void writeEnumsInitialization(TextStream &s, AbstractMetaEnumList &enums) const; void writeEnumInitialization(TextStream &s, const AbstractMetaEnum &metaEnum) const; diff --git a/sources/shiboken6/tests/libsmart/smart.cpp b/sources/shiboken6/tests/libsmart/smart.cpp index 81fa30c7e..6908c7881 100644 --- a/sources/shiboken6/tests/libsmart/smart.cpp +++ b/sources/shiboken6/tests/libsmart/smart.cpp @@ -137,6 +137,18 @@ int Obj::takeSharedPtrToInteger(SharedPtr pInt) return pInt->value(); } +SharedPtr Obj::createSharedPtrToInteger(int value) +{ + auto *i = new Integer; + i->setValue(value); + return SharedPtr(i); +} + +SharedPtr Obj::createNullSharedPtrToInteger() +{ + return {}; +} + SharedPtr Obj::giveSharedPtrToConstInteger() { SharedPtr co(new Integer); @@ -194,6 +206,13 @@ void Integer::setValue(int v) m_int = v; } +int Integer::compare(const Integer &rhs) const +{ + if (m_int < rhs.m_int) + return -1; + return m_int > rhs.m_int ? 1 : 0; +} + void Integer::printInteger() const { if (shouldPrint()) diff --git a/sources/shiboken6/tests/libsmart/smart_integer.h b/sources/shiboken6/tests/libsmart/smart_integer.h index 126894120..0f2dfcec9 100644 --- a/sources/shiboken6/tests/libsmart/smart_integer.h +++ b/sources/shiboken6/tests/libsmart/smart_integer.h @@ -42,9 +42,41 @@ public: int value() const; void setValue(int v); + int compare(const Integer &rhs) const; + int m_int; // public for testing member field access. }; +inline bool operator==(const Integer &lhs, const Integer &rhs) +{ + return lhs.compare(rhs) == 0; +} + +inline bool operator!=(const Integer &lhs, const Integer &rhs) +{ + return lhs.compare(rhs) != 0; +} + +inline bool operator<(const Integer &lhs, const Integer &rhs) +{ + return lhs.compare(rhs) < 0; +} + +inline bool operator<=(const Integer &lhs, const Integer &rhs) +{ + return lhs.compare(rhs) <= 0; +} + +inline bool operator>(const Integer &lhs, const Integer &rhs) +{ + return lhs.compare(rhs) > 0; +} + +inline bool operator>=(const Integer &lhs, const Integer &rhs) +{ + return lhs.compare(rhs) >= 0; +} + namespace Smart { class LIB_SMART_API Integer2 : public Integer { public: diff --git a/sources/shiboken6/tests/libsmart/smart_obj.h b/sources/shiboken6/tests/libsmart/smart_obj.h index 8fe45993f..9cd8e5f08 100644 --- a/sources/shiboken6/tests/libsmart/smart_obj.h +++ b/sources/shiboken6/tests/libsmart/smart_obj.h @@ -46,7 +46,7 @@ public: void printObj(); Integer takeInteger(Integer val); - SharedPtr giveSharedPtrToObj(); + static SharedPtr giveSharedPtrToObj(); std::vector > giveSharedPtrToObjList(int size); virtual SharedPtr giveSharedPtrToInteger(); // virtual for PYSIDE-1188 SharedPtr giveSharedPtrToConstInteger(); @@ -55,6 +55,9 @@ public: int takeSharedPtrToObj(SharedPtr pObj); int takeSharedPtrToInteger(SharedPtr pInt); + static SharedPtr createSharedPtrToInteger(int value); + static SharedPtr createNullSharedPtrToInteger(); + int m_integer; // public for testing member field access. Integer *m_internalInteger; }; diff --git a/sources/shiboken6/tests/smartbinding/smart_pointer_test.py b/sources/shiboken6/tests/smartbinding/smart_pointer_test.py index 10e761149..ce976e1ca 100644 --- a/sources/shiboken6/tests/smartbinding/smart_pointer_test.py +++ b/sources/shiboken6/tests/smartbinding/smart_pointer_test.py @@ -239,5 +239,39 @@ class SmartPointerTests(unittest.TestCase): r = o.takeSharedPtrToInteger(integer2) self.assertEqual(r, integer2.value()) + def testSmartPointerValueComparison(self): + """Test a pointee class with comparison operators.""" + four = Obj.createSharedPtrToInteger(4) + four2 = Obj.createSharedPtrToInteger(4) + five = Obj.createSharedPtrToInteger(5) + self.assertTrue(four == four) + self.assertTrue(four == four2) + self.assertFalse(four != four) + self.assertFalse(four != four2) + self.assertFalse(four < four) + self.assertTrue(four <= four) + self.assertFalse(four > four) + self.assertTrue(four >= four) + self.assertFalse(four == five) + self.assertTrue(four != five) + self.assertTrue(four < five) + self.assertTrue(four <= five) + self.assertFalse(four > five) + self.assertFalse(four >= five) + self.assertTrue(five > four) + + self.assertRaises(NotImplementedError, + lambda : Obj.createNullSharedPtrToInteger() == four) + + def testSmartPointerObjectComparison(self): + """Test a pointee class without comparison operators.""" + o1 = Obj.giveSharedPtrToObj() + o2 = Obj.giveSharedPtrToObj() + self.assertTrue(o1 == o1) + self.assertFalse(o1 != o1) + self.assertFalse(o1 == o2) + self.assertTrue(o1 != o2) + + if __name__ == '__main__': unittest.main() -- cgit v1.2.3