From bfacabede1f9cb7dc1747879b310c4855b65bccf Mon Sep 17 00:00:00 2001 From: Friedemann Kleint Date: Tue, 1 Dec 2020 15:23:20 +0100 Subject: shiboken6: Fix field modifications The logic was only partially present, neither removal of getter/setter nor renaming had any effect. Rewrite the code to resemble that of AbstractMetaFunction (adding function applyFieldModifications()) Move some check functions from the generators to AbstractMetaField/Type. Add tests in libsample. Change-Id: Ib29d4e37db51f122b46702cb5d96b13da6d0f224 Reviewed-by: Christian Tismer --- .../shiboken6/ApiExtractor/abstractmetabuilder.cpp | 28 ++++++++++-- .../shiboken6/ApiExtractor/abstractmetafield.cpp | 52 +++++++++++++++++----- sources/shiboken6/ApiExtractor/abstractmetafield.h | 12 ++++- .../shiboken6/ApiExtractor/abstractmetatype.cpp | 6 +++ sources/shiboken6/ApiExtractor/abstractmetatype.h | 3 ++ .../shiboken6/ApiExtractor/typesystemparser.cpp | 3 ++ .../shiboken6/generator/shiboken/cppgenerator.cpp | 37 +++++---------- sources/shiboken6/tests/libsample/abstract.cpp | 2 +- sources/shiboken6/tests/libsample/abstract.h | 2 + .../tests/samplebinding/class_fields_test.py | 21 +++++++++ .../tests/samplebinding/typesystem_sample.xml | 2 + 11 files changed, 124 insertions(+), 44 deletions(-) diff --git a/sources/shiboken6/ApiExtractor/abstractmetabuilder.cpp b/sources/shiboken6/ApiExtractor/abstractmetabuilder.cpp index 0d86e028f..ac410e920 100644 --- a/sources/shiboken6/ApiExtractor/abstractmetabuilder.cpp +++ b/sources/shiboken6/ApiExtractor/abstractmetabuilder.cpp @@ -1175,15 +1175,35 @@ std::optional return metaField; } +static bool applyFieldModifications(AbstractMetaField *f) +{ + const auto &modifications = f->modifications(); + for (const auto &mod : modifications) { + if (mod.isRemoveModifier() && mod.removal() == TypeSystem::All) + return false; + if (mod.isRenameModifier()) { + f->setOriginalName(f->name()); + f->setName(mod.renamedToName()); + } else if (!mod.isReadable()) { + f->setGetterEnabled(false); + } else if (!mod.isWritable()) { + f->setSetterEnabled(false); + } + } + f->setOriginalAttributes(f->attributes()); + return true; +} + void AbstractMetaBuilderPrivate::traverseFields(const ScopeModelItem &scope_item, AbstractMetaClass *metaClass) { const VariableList &variables = scope_item->variables(); for (const VariableModelItem &field : variables) { - auto metaField = traverseField(field, metaClass); - if (metaField.has_value() && !metaField->isModifiedRemoved()) { - metaField->setOriginalAttributes(metaField->attributes()); - metaClass->addField(*metaField); + auto metaFieldO = traverseField(field, metaClass); + if (metaFieldO.has_value()) { + AbstractMetaField metaField = metaFieldO.value(); + if (applyFieldModifications(&metaField)) + metaClass->addField(metaField); } } } diff --git a/sources/shiboken6/ApiExtractor/abstractmetafield.cpp b/sources/shiboken6/ApiExtractor/abstractmetafield.cpp index 841266a37..fa7ab64d9 100644 --- a/sources/shiboken6/ApiExtractor/abstractmetafield.cpp +++ b/sources/shiboken6/ApiExtractor/abstractmetafield.cpp @@ -41,8 +41,9 @@ public: QString m_originalName; QString m_name; AbstractMetaType m_type; - bool m_hasName = false; Documentation m_doc; + bool m_setterEnabled = true; // Modifications + bool m_getterEnabled = true; // Modifications }; AbstractMetaField::AbstractMetaField() : d(new AbstractMetaFieldData) @@ -101,27 +102,21 @@ QString AbstractMetaField::name() const return d->m_name; } -void AbstractMetaField::setName(const QString &name, bool realName) +void AbstractMetaField::setName(const QString &name) { - if (d->m_name != name || d->m_hasName != realName) { + if (d->m_name != name) d->m_name = name; - d->m_hasName = realName; - } -} - -bool AbstractMetaField::hasName() const -{ - return d->m_hasName; } QString AbstractMetaField::qualifiedCppName() const { - return enclosingClass()->qualifiedCppName() + QLatin1String("::") + d->m_name; + return enclosingClass()->qualifiedCppName() + QLatin1String("::") + + originalName(); } QString AbstractMetaField::originalName() const { - return d->m_originalName; + return d->m_originalName.isEmpty() ? d->m_name : d->m_originalName; } void AbstractMetaField::setOriginalName(const QString &name) @@ -141,6 +136,39 @@ void AbstractMetaField::setDocumentation(const Documentation &doc) d->m_doc = doc; } +bool AbstractMetaField::isGetterEnabled() const +{ + return d->m_getterEnabled; +} + +void AbstractMetaField::setGetterEnabled(bool e) +{ + if (d->m_getterEnabled != e) + d->m_getterEnabled = e; +} + +bool AbstractMetaField::isSetterEnabled() const +{ + return d->m_setterEnabled; +} + +void AbstractMetaField::setSetterEnabled(bool e) +{ + if (e != d->m_setterEnabled) + d->m_setterEnabled = e; +} + +bool AbstractMetaField::canGenerateGetter() const +{ + return d->m_getterEnabled && !isStatic(); +} + +bool AbstractMetaField::canGenerateSetter() const +{ + return d->m_setterEnabled && !isStatic() + && (!d->m_type.isConstant() || d->m_type.isPointerToConst()); +} + FieldModificationList AbstractMetaField::modifications() const { const FieldModificationList &mods = enclosingClass()->typeEntry()->fieldModifications(); diff --git a/sources/shiboken6/ApiExtractor/abstractmetafield.h b/sources/shiboken6/ApiExtractor/abstractmetafield.h index ccd597471..7964d5b57 100644 --- a/sources/shiboken6/ApiExtractor/abstractmetafield.h +++ b/sources/shiboken6/ApiExtractor/abstractmetafield.h @@ -62,8 +62,8 @@ public: void setType(const AbstractMetaType &type); QString name() const; - void setName(const QString &name, bool realName = true); - bool hasName() const; + void setName(const QString &name); + QString qualifiedCppName() const; QString originalName() const; @@ -72,6 +72,14 @@ public: const Documentation &documentation() const; void setDocumentation(const Documentation& doc); + bool isGetterEnabled() const; // Modifications + void setGetterEnabled(bool e); + bool isSetterEnabled() const; // Modifications + void setSetterEnabled(bool e); + + bool canGenerateGetter() const; + bool canGenerateSetter() const; + static std::optional find(const AbstractMetaFieldList &haystack, const QString &needle); diff --git a/sources/shiboken6/ApiExtractor/abstractmetatype.cpp b/sources/shiboken6/ApiExtractor/abstractmetatype.cpp index fa28238f2..990d1ec07 100644 --- a/sources/shiboken6/ApiExtractor/abstractmetatype.cpp +++ b/sources/shiboken6/ApiExtractor/abstractmetatype.cpp @@ -689,6 +689,12 @@ bool AbstractMetaType::isPointer() const || isNativePointer() || isValuePointer(); } +bool AbstractMetaType::isPointerToConst() const +{ + return d->m_constant && !d->m_indirections.isEmpty() + && d->m_indirections.constLast() != Indirection::ConstPointer; +} + bool AbstractMetaType::isCString() const { return isNativePointer() diff --git a/sources/shiboken6/ApiExtractor/abstractmetatype.h b/sources/shiboken6/ApiExtractor/abstractmetatype.h index a65858364..599321262 100644 --- a/sources/shiboken6/ApiExtractor/abstractmetatype.h +++ b/sources/shiboken6/ApiExtractor/abstractmetatype.h @@ -197,6 +197,9 @@ public: // Query functions for generators /// Check if type is a pointer. bool isPointer() const; + /// Helper for field setters: Check for "const QWidget *" (settable field), + /// but not "int *const" (read-only field). + bool isPointerToConst() const; /// Returns true if the type is a C string (const char *). bool isCString() const; /// Returns true if the type is a void pointer. diff --git a/sources/shiboken6/ApiExtractor/typesystemparser.cpp b/sources/shiboken6/ApiExtractor/typesystemparser.cpp index 209d88290..b56fef22a 100644 --- a/sources/shiboken6/ApiExtractor/typesystemparser.cpp +++ b/sources/shiboken6/ApiExtractor/typesystemparser.cpp @@ -2165,6 +2165,9 @@ bool TypeSystemParser::parseModifyField(const QXmlStreamReader &reader, qPrintable(msgUnimplementedAttributeWarning(reader, name))); if (!convertBoolean(attributes->takeAt(i).value(), writeAttribute(), true)) fm.clearModifierFlag(FieldModification::Writable); + } else if (name == renameAttribute()) { + fm.setRenamedToName(attributes->takeAt(i).value().toString()); + fm.setModifierFlag(Modification::Rename); } } if (fm.name.isEmpty()) { diff --git a/sources/shiboken6/generator/shiboken/cppgenerator.cpp b/sources/shiboken6/generator/shiboken/cppgenerator.cpp index 7fdea833f..48f7915be 100644 --- a/sources/shiboken6/generator/shiboken/cppgenerator.cpp +++ b/sources/shiboken6/generator/shiboken/cppgenerator.cpp @@ -291,21 +291,6 @@ static QString chopType(QString s) return s; } -// Helper for field setters: Check for "const QWidget *" (settable field), -// but not "int *const" (read-only field). -static bool isPointerToConst(const AbstractMetaType &t) -{ - const AbstractMetaType::Indirections &indirections = t.indirectionsV(); - return t.isConstant() && !indirections.isEmpty() - && indirections.constLast() != Indirection::ConstPointer; -} - -static inline bool canGenerateFieldSetter(const AbstractMetaField &field) -{ - const AbstractMetaType &type = field.type(); - return !type.isConstant() || isPointerToConst(type); -} - static bool isStdSetterName(QString setterName, QString propertyName) { return setterName.size() == propertyName.size() + 3 @@ -712,10 +697,9 @@ void CppGenerator::generateClass(TextStream &s, const GeneratorContext &classCon if (shouldGenerateGetSetList(metaClass) && !classContext.forSmartPointer()) { const AbstractMetaFieldList &fields = metaClass->fields(); for (const AbstractMetaField &metaField : fields) { - if (metaField.isStatic()) - continue; - writeGetterFunction(s, metaField, classContext); - if (canGenerateFieldSetter(metaField)) + if (metaField.canGenerateGetter()) + writeGetterFunction(s, metaField, classContext); + if (metaField.canGenerateSetter()) writeSetterFunction(s, metaField, classContext); s << '\n'; } @@ -733,10 +717,11 @@ void CppGenerator::generateClass(TextStream &s, const GeneratorContext &classCon << "[] = {\n" << indent; for (const AbstractMetaField &metaField : fields) { if (!metaField.isStatic()) { - const QString setter = canGenerateFieldSetter(metaField) + const QString getter = metaField.canGenerateGetter() + ? cpythonGetterFunctionName(metaField) : QString(); + const QString setter = metaField.canGenerateSetter() ? cpythonSetterFunctionName(metaField) : QString(); - writePyGetSetDefEntry(s, metaField.name(), - cpythonGetterFunctionName(metaField), setter); + writePyGetSetDefEntry(s, metaField.name(), getter, setter); } } @@ -4583,7 +4568,8 @@ void CppGenerator::writeGetterFunction(TextStream &s, << context.wrapperName() << " *>(" << CPP_SELF_VAR << ")->" << protectedFieldGetterName(metaField) << "()"; } else { - cppField = QLatin1String(CPP_SELF_VAR) + QLatin1String("->") + metaField.name(); + cppField = QLatin1String(CPP_SELF_VAR) + QLatin1String("->") + + metaField.originalName(); if (newWrapperSameObject) { cppField.prepend(QLatin1String("&(")); cppField.append(QLatin1Char(')')); @@ -4704,7 +4690,8 @@ void CppGenerator::writeSetterFunction(TextStream &s, fieldType, context); - QString cppField = QLatin1String(CPP_SELF_VAR) + QLatin1String("->") + metaField.name(); + const QString cppField = QLatin1String(CPP_SELF_VAR) + QLatin1String("->") + + metaField.originalName(); if (avoidProtectedHack() && metaField.isProtected()) { s << getFullTypeNameWithoutModifiers(fieldType); if (fieldType.indirections() == 1) @@ -4721,7 +4708,7 @@ void CppGenerator::writeSetterFunction(TextStream &s, << PYTHON_TO_CPP_VAR << "(pyIn, &cppOut_local);\n" << cppField << " = cppOut_local"; } else { - if (isPointerToConst(fieldType)) + if (fieldType.isPointerToConst()) s << "const "; s << getFullTypeNameWithoutModifiers(fieldType) << QString::fromLatin1(" *").repeated(fieldType.indirections()) << "& cppOut_ptr = " diff --git a/sources/shiboken6/tests/libsample/abstract.cpp b/sources/shiboken6/tests/libsample/abstract.cpp index e60c792c4..3b2b1ef0a 100644 --- a/sources/shiboken6/tests/libsample/abstract.cpp +++ b/sources/shiboken6/tests/libsample/abstract.cpp @@ -36,7 +36,7 @@ const int Abstract::staticPrimitiveField = 0; Abstract::Abstract(int id) : m_id(id) { - primitiveField = 123; + toBeRenamedField = readOnlyField = primitiveField = 123; valueTypeField = Point(12, 34); objectTypeField = nullptr; bitField = 0; diff --git a/sources/shiboken6/tests/libsample/abstract.h b/sources/shiboken6/tests/libsample/abstract.h index 09906f1ee..746107c71 100644 --- a/sources/shiboken6/tests/libsample/abstract.h +++ b/sources/shiboken6/tests/libsample/abstract.h @@ -67,6 +67,8 @@ public: Complex userPrimitiveField; Point valueTypeField; ObjectType* objectTypeField; + int toBeRenamedField; + int readOnlyField; Abstract(int id = -1); virtual ~Abstract(); diff --git a/sources/shiboken6/tests/samplebinding/class_fields_test.py b/sources/shiboken6/tests/samplebinding/class_fields_test.py index 202efcafb..9cc59762e 100644 --- a/sources/shiboken6/tests/samplebinding/class_fields_test.py +++ b/sources/shiboken6/tests/samplebinding/class_fields_test.py @@ -65,6 +65,27 @@ class TestAccessingCppFields(unittest.TestCase): # attribution with invalid type self.assertRaises(TypeError, lambda : setattr(d, 'primitiveField', None)) + def testAccessingRenamedFields(self): + '''Reads and writes a renamed field.''' + d = Derived() + self.assertEqual(type(d.renamedField), int) + old_value = d.renamedField + new_value = 2255 + d.renamedField = new_value + self.assertEqual(d.renamedField, new_value) + self.assertNotEqual(d.renamedField, old_value) + + def testAccessingReadOnlyFields(self): + '''Tests a read-only field.''' + d = Derived() + self.assertEqual(type(d.readOnlyField), int) + old_value = d.readOnlyField + try: + d.readOnlyField = 25555 + except AttributeError: + pass + self.assertEqual(d.readOnlyField, old_value) + def testAccessingUsersPrimitiveTypeField(self): '''Reads and writes an user's primitive type (in this case an 'Complex') field.''' d = Derived() diff --git a/sources/shiboken6/tests/samplebinding/typesystem_sample.xml b/sources/shiboken6/tests/samplebinding/typesystem_sample.xml index e1c5ab74a..6d9c842ac 100644 --- a/sources/shiboken6/tests/samplebinding/typesystem_sample.xml +++ b/sources/shiboken6/tests/samplebinding/typesystem_sample.xml @@ -640,6 +640,8 @@ + + -- cgit v1.2.3