-
Notifications
You must be signed in to change notification settings - Fork 137
[ffigen] Add C++ public inheritance support (single, multiple, diamond) #3542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
a95321d
4e79ab9
71af05a
7c3a325
a600f41
06d61f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
| import '../code_generator.dart'; | ||
| import '../config_provider/public_ast.dart' as public_ast; | ||
| import '../context.dart'; | ||
| import '../header_parser/sub_parsers/classdecl_parser.dart' | ||
| show InheritedMethod, collectInheritedMethods, methodSignatureKey; | ||
| import '../visitor/ast.dart'; | ||
|
|
||
| import 'binding_string.dart'; | ||
|
|
@@ -71,6 +73,12 @@ class CppClass extends BindingType with HasLocalScope { | |
| final List<CppMethod> methods; | ||
| final List<CppMember> fields; | ||
|
|
||
| /// The public C++ base classes for this class, in declaration order. | ||
| /// | ||
| /// Only public inheritance is represented here. Protected and private bases | ||
| /// are silently ignored by the parser. | ||
| final List<CppClass> bases; | ||
|
|
||
| CppClass({ | ||
| super.usr, | ||
| super.originalName, | ||
|
|
@@ -79,6 +87,7 @@ class CppClass extends BindingType with HasLocalScope { | |
| required this.context, | ||
| required this.methods, | ||
| required this.fields, | ||
| this.bases = const [], | ||
| }); | ||
|
|
||
| @override | ||
|
|
@@ -96,6 +105,20 @@ class CppClass extends BindingType with HasLocalScope { | |
| required LocalVariables localVariables, | ||
| }) => '$value._ptr'; | ||
|
|
||
| /// Returns the list of inherited methods from base classes that are not | ||
| /// overridden by this class. | ||
| List<InheritedMethod> getInheritedMethodsToDelegate(Context ctx) { | ||
| if (bases.isEmpty) return const []; | ||
| final ownSignatures = methods | ||
| .map((m) => methodSignatureKey(m, ctx)) | ||
| .toSet(); | ||
| return collectInheritedMethods(this) | ||
| .where( | ||
| (im) => !ownSignatures.contains(methodSignatureKey(im.method, ctx)), | ||
| ) | ||
| .toList(); | ||
| } | ||
|
|
||
| @override | ||
| BindingString toBindingString(Writer w) { | ||
| final s = StringBuffer(); | ||
|
|
@@ -115,8 +138,13 @@ class CppClass extends BindingType with HasLocalScope { | |
| final deleteGlue = '_$deleteSymbol'; | ||
|
|
||
| s.write(makeDartDoc(dartDoc)); | ||
| // Build the implements clause: ffi.Finalizable + public base classes. | ||
| final baseNames = bases.map((b) => b.name).join(', '); | ||
| final implementsClause = bases.isEmpty | ||
| ? '$ffiPrefix.Finalizable' | ||
| : '$ffiPrefix.Finalizable, $baseNames'; | ||
| s.write(''' | ||
| class $name implements $ffiPrefix.Finalizable { | ||
| class $name implements $implementsClause { | ||
| $ptrVoid _ptr; | ||
| '''); | ||
|
|
||
|
|
@@ -287,7 +315,7 @@ class $name implements $ffiPrefix.Finalizable { | |
| } | ||
| } | ||
| s.write(''' | ||
| void dispose() { | ||
| ${bases.isNotEmpty ? '@override\n ' : ''}void dispose() { | ||
| if (_ptr == $ffiPrefix.nullptr) { | ||
| throw StateError('This object has already been disposed.'); | ||
| } | ||
|
|
@@ -302,6 +330,49 @@ class $name implements $ffiPrefix.Finalizable { | |
| _ptr = $ffiPrefix.nullptr; | ||
| } | ||
| '''); | ||
|
|
||
| // Inherited method delegation (Dart side) | ||
| final inheritedToDelegate = getInheritedMethodsToDelegate(ctx); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than have this huge amount of duplicated code, just add the super type's methods to this class's methods. That way there's nothing special about these methods at all. It probably makes sense to do that in lib/src/visitor/copy_methods_from_super_type.dart. When copying across the method, make an actual copy of it, like we do for ObjC methods.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Could you check if this is what you had in mind, or if there are any further improvements needed? |
||
| for (final im in inheritedToDelegate) { | ||
| final method = im.method; | ||
| final base = im.baseClass; | ||
| final delegateSymbol = '${name}_${base.name}_${method.originalName}'; | ||
| final delegateGlue = '_$delegateSymbol'; | ||
| final dartReturn = method.returnType.getDartType(ctx); | ||
| final dartParams = dartParamList(method.parameters); | ||
| final localVars = LocalVariables(method.localScope); | ||
| final callArgs = [ | ||
| '_ptr', | ||
| ...method.parameters.map( | ||
| (p) => p.type.convertDartTypeToFfiDartType( | ||
| ctx, | ||
| p.name, | ||
| objCRetain: false, | ||
| objCAutorelease: false, | ||
| localVariables: localVars, | ||
| ), | ||
| ), | ||
| ].join(', '); | ||
| final decls = localVars.generateDeclarations(); | ||
| final returnExpr = method.returnType.convertFfiDartTypeToDartType( | ||
| ctx, | ||
| '$delegateGlue($callArgs)', | ||
| objCRetain: false, | ||
| ); | ||
| final hasReturn = method.returnType != voidType; | ||
| final callLine = hasReturn ? 'return $returnExpr;' : '$returnExpr;'; | ||
| s.write('''\ | ||
| @override | ||
| $dartReturn ${method.originalName}($dartParams) { | ||
| if (_ptr == $ffiPrefix.nullptr) { | ||
| throw StateError('This object has already been disposed.'); | ||
| } | ||
| $decls | ||
| $callLine | ||
| } | ||
| '''); | ||
| } | ||
|
|
||
| s.write('}\n'); | ||
|
|
||
| // Writes a @Native annotation + external declaration for a glue function. | ||
|
|
@@ -362,6 +433,33 @@ class $name implements $ffiPrefix.Finalizable { | |
| ffiParams: '$ptrVoid self', | ||
| ); | ||
|
|
||
| // @Native declarations for inherited-method delegation glue | ||
| for (final im in inheritedToDelegate) { | ||
| final method = im.method; | ||
| final base = im.baseClass; | ||
| final delegateSymbol = '${name}_${base.name}_${method.originalName}'; | ||
| final delegateGlue = '_$delegateSymbol'; | ||
| final cReturn = method.returnType.getCType(ctx); | ||
| final ffiReturn = method.returnType.getFfiDartType(ctx); | ||
| final cParams = [ | ||
| ptrVoid, // self (typed as derived) | ||
| ...method.parameters.map((p) => p.type.getCType(ctx)), | ||
| ].join(', '); | ||
| final ffiParams = [ | ||
| '$ptrVoid self', | ||
| ...method.parameters.map( | ||
| (p) => '${p.type.getFfiDartType(ctx)} ${p.name}', | ||
| ), | ||
| ].join(', '); | ||
| writeNativeDecl( | ||
| symbol: delegateSymbol, | ||
| glue: delegateGlue, | ||
| cType: '$cReturn Function($cParams)', | ||
| ffiReturn: ffiReturn, | ||
| ffiParams: ffiParams, | ||
| ); | ||
| } | ||
|
|
||
| return BindingString( | ||
| type: BindingStringType.cppClass, | ||
| string: s.toString(), | ||
|
|
@@ -408,12 +506,8 @@ FFIGEN_EXPORT void ${name}_delete($originalName* self) { | |
| '$returnPrefix$originalName::' | ||
| '${method.originalName}($callArgs);'; | ||
| } else { | ||
| final String selfType; | ||
| if (method.isConstant) { | ||
| selfType = 'const $originalName'; | ||
| } else { | ||
| selfType = originalName; | ||
| } | ||
| final constPrefix = method.isConstant ? 'const ' : ''; | ||
| final selfType = '$constPrefix$originalName'; | ||
| params = ['$selfType* self', ...otherParams].join(', '); | ||
| final methodName = method.originalName; | ||
| final suffix = method.returnType is CppUniquePtrType | ||
|
|
@@ -430,6 +524,44 @@ FFIGEN_EXPORT $returnTypeString $symbol($params) { | |
| }) | ||
| .join('\n\n'); | ||
|
|
||
| // Delegation stubs for inherited methods (C++ side) | ||
| final inheritedBindings = StringBuffer(); | ||
| final inheritedToDelegate = getInheritedMethodsToDelegate(context); | ||
| for (final im in inheritedToDelegate) { | ||
| final method = im.method; | ||
| final base = im.baseClass; | ||
| final delegateSymbol = | ||
| '${name}_${base.originalName}_${method.originalName}'; | ||
| final callArgs = method.parameters.map(_cppCallArg).join(', '); | ||
|
|
||
| final nativeType = method.returnType.getNativeType(context); | ||
| final returnTypeString = nativeType.trim(); | ||
| final needsReturn = method.returnType != voidType; | ||
| final returnPrefix = needsReturn ? 'return ' : ''; | ||
| final suffix = method.returnType is CppUniquePtrType ? '.release()' : ''; | ||
|
|
||
| final constPrefix = method.isConstant ? 'const ' : ''; | ||
| final selfType = '$constPrefix$originalName'; | ||
| final otherParams = method.parameters.map(paramDecl); | ||
| final params = ['$selfType* self', ...otherParams].join(', '); | ||
|
|
||
| // static_cast adjusts the this-pointer offset for the base sub-object. | ||
| final castTarget = 'static_cast<$constPrefix${base.originalName}*>(self)'; | ||
| final body = | ||
| '$returnPrefix$castTarget' | ||
| '->${method.originalName}($callArgs)$suffix;'; | ||
|
|
||
| inheritedBindings.write(''' | ||
|
|
||
| FFIGEN_EXPORT $returnTypeString $delegateSymbol($params) { | ||
| $body | ||
| }'''); | ||
| } | ||
|
|
||
| if (inheritedBindings.isNotEmpty) { | ||
| return '$methodBindings\n\n$deleteWrapper\n' | ||
| '${inheritedBindings.toString()}\n\n'; | ||
| } | ||
| return '$methodBindings\n\n$deleteWrapper\n\n'; | ||
| } | ||
|
|
||
|
|
@@ -456,6 +588,7 @@ FFIGEN_EXPORT $returnTypeString $symbol($params) { | |
| super.visitChildren(visitor); | ||
| visitor.visitAll(methods); | ||
| visitor.visitAll(fields); | ||
| visitor.visitAll(bases); | ||
| visitor.visit(ffiImport); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,9 @@ CppClass? parseClassDeclaration(Context context, clang_types.CXCursor cursor) { | |
| } | ||
| }); | ||
|
|
||
| // Parse public base classes (only public specifiers; non-public are ignored). | ||
| final bases = _parsePublicBases(context, cursor); | ||
|
|
||
| final cppClass = CppClass( | ||
| usr: usr, | ||
| dartDoc: getCursorDocComment( | ||
|
|
@@ -76,13 +79,84 @@ CppClass? parseClassDeclaration(Context context, clang_types.CXCursor cursor) { | |
| context: context, | ||
| methods: methods, | ||
| fields: <CppMember>[], | ||
| bases: bases, | ||
| ); | ||
|
|
||
| context.bindingsIndex.addCppClassToSeen(usr, cppClass); | ||
|
|
||
| return cppClass; | ||
| } | ||
|
|
||
| /// Parses the direct public base classes of [cursor]. | ||
| List<CppClass> _parsePublicBases(Context context, clang_types.CXCursor cursor) { | ||
| final bases = <CppClass>[]; | ||
|
|
||
| cursor.visitChildren((child) { | ||
| final kind = clang.clang_getCursorKind(child); | ||
| if (kind != clang_types.CXCursorKind.CXCursor_CXXBaseSpecifier) return; | ||
|
|
||
| final access = clang.clang_getCXXAccessSpecifier(child); | ||
| if (access != clang_types.CX_CXXAccessSpecifier.CX_CXXPublic) return; | ||
|
|
||
| final baseType = clang.clang_getCursorType(child); | ||
| final baseDeclCursor = clang.clang_getTypeDeclaration(baseType); | ||
| final baseUsr = baseDeclCursor.usr(); | ||
|
|
||
| final baseClass = context.bindingsIndex.getSeenCppClass(baseUsr); | ||
| if (baseClass == null) { | ||
| final parsed = parseClassDeclaration(context, baseDeclCursor); | ||
| if (parsed != null) bases.add(parsed); | ||
| } else { | ||
| bases.add(baseClass); | ||
| } | ||
| }); | ||
|
|
||
| return bases; | ||
| } | ||
|
|
||
| String methodSignatureKey(CppMethod method, Context context) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a method on |
||
| final paramTypes = method.parameters | ||
| .map((p) => p.type.getNativeType(context)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's the kind of bug that you only catch if you have very good test coverage. Eg, writing a C++ class with a method for every possible return type and arg type (primitives, structs, unions, classes, function pointers etc etc etc). You should use |
||
| .join(','); | ||
| final constSuffix = method.isConstant ? ' const' : ''; | ||
| return '${method.originalName}($paramTypes)$constSuffix'; | ||
| } | ||
|
|
||
| List<InheritedMethod> collectInheritedMethods(CppClass cls) { | ||
| final seen = <String>{}; | ||
| final result = <InheritedMethod>[]; | ||
| for (final directBase in cls.bases) { | ||
| _collectFromBase(directBase, directBase, seen, result, cls.context); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| void _collectFromBase( | ||
| CppClass current, | ||
| CppClass directBase, | ||
| Set<String> seen, | ||
| List<InheritedMethod> result, | ||
| Context context, | ||
| ) { | ||
| for (final base in current.bases) { | ||
| _collectFromBase(base, directBase, seen, result, context); | ||
| } | ||
| for (final method in current.methods) { | ||
| if (method.kind == CppMethodKind.constructor) continue; | ||
| final key = methodSignatureKey(method, context); | ||
| if (seen.add(key)) { | ||
| result.add(InheritedMethod(method: method, baseClass: directBase)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class InheritedMethod { | ||
| final CppMethod method; | ||
| final CppClass baseClass; | ||
|
|
||
| const InheritedMethod({required this.method, required this.baseClass}); | ||
| } | ||
|
|
||
| void _parseAnyMethod( | ||
| Context context, | ||
| clang_types.CXCursor cursor, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if you combine
'$ffiPrefix.Finalizable'into a list literal with the iterable returned bybases.map, then you can just rely on the.joinand don't need this conditional.