[ffigen] Add C++ public inheritance support (single, multiple, diamond) - #3542
[ffigen] Add C++ public inheritance support (single, multiple, diamond)#3542Hassnaa9 wants to merge 6 commits into
Conversation
| 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 |
There was a problem hiding this comment.
nit: if you combine '$ffiPrefix.Finalizable' into a list literal with the iterable returned by bases.map, then you can just rely on the .join and don't need this conditional.
| '''); | ||
|
|
||
| // Inherited method delegation (Dart side) | ||
| final inheritedToDelegate = getInheritedMethodsToDelegate(ctx); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done. Could you check if this is what you had in mind, or if there are any further improvements needed?
| public: | ||
| DiamondBase(); | ||
| virtual ~DiamondBase(); | ||
| int baseVal() const; |
There was a problem hiding this comment.
Add a virtual method test?
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
Looks like there's some formatting errors on CI. The bot is using the most recent stable Dart, which was just released (and makes some small changes to the formatter), so make sure you update to the latest version.
| final bool isConstant; | ||
| final bool isStatic; | ||
| final CppMethodKind kind; | ||
| final String? originatingClass; |
There was a problem hiding this comment.
Better to store a reference to the CppClass object.
| return bases; | ||
| } | ||
|
|
||
| String methodSignatureKey(CppMethod method, Context context) { |
There was a problem hiding this comment.
This should be a method on CppMethod.
| CppMethod cloneForClass(CppClass targetClass, CppClass baseClass) { | ||
| return CppMethod( | ||
| name: Symbol( | ||
| '${targetClass.originalName}_$originalName', |
There was a problem hiding this comment.
I was pretty confused about this name mangling until I realised that you're using originalName instead of name when generating the Dart method. That means that users won't be able to rename methods, and the name collision resolution logic won't work.
You don't need to fix that in this PR, but I filed a bug so we don't forget: #3552
| if (method.originatingClass != null) { | ||
| final origClass = method.originatingClass; | ||
| final castTarget = | ||
| 'static_cast<$constPrefix$origClass*>(self)'; |
There was a problem hiding this comment.
Why is the static_cast necessary?
There was a problem hiding this comment.
The cast is used to convert the derived self pointer to the base-class subobject that the inherited method belongs to. which is important for multiple inheritance, where the base subobject may have a different this pointer offset. The cast lets C++ perform that adjustment while preserving normal virtual dispatch.
There was a problem hiding this comment.
I don't understand. That explanation doesn't really make sense. A cast like that isn't necessary in pure C++ code, so it shouldn't be necessary here. And casting here is problematic for the same reason I mentioned elsewhere: we shouldn't be making dispatching decisions ourselves (that's the C++ language's job). Multiple inheritance ambiguities are handled using different syntax (that's what this bug is about).
Is there a test that fails if you don't do this? If not, you should try to write one, to prove that the cast is necessary.
There was a problem hiding this comment.
@liamappelbe I removed the static_cast and ran the existing inheritance tests. This causes the generated C++ glue to fail to compile for our diamond inheritance case
error: non-static member 'baseVal' found in multiple base-class subobjects of type 'DiamondBase'
class DiamondDerived -> DiamondLeft -> DiamondBase
class DiamondDerived -> DiamondRight -> DiamondBase
I agree that maybe the cast itself not be the right way to express that. I think this is the same issue covered by #3544, so I'll avoid using the cast as a general dispatch mechanism and leave the diamond specific handling for that issue?
There was a problem hiding this comment.
Yeah, there's 2 ways of fixing that. One is to disambiguate using explicit scoping (that's what #3544 talks about). The other way is to change this test to use virtual inheritance:
class DerivedA : virtual public DiamondBase {...};
class DerivedB : virtual public DiamondBase {...};It's not a universal solution to the problem because if the user doesn't own the API, they can't update it to use virtual inheritance. But it's fine as a workaround for this test. Use virtual inheritance for this test, to avoid dealing with #3544 for now.
|
|
||
| String methodSignatureKey(CppMethod method, Context context) { | ||
| final paramTypes = method.parameters | ||
| .map((p) => p.type.getNativeType(context)) |
There was a problem hiding this comment.
getNativeType is only supposed to be used at codegen time. In general, these getters that are used in codegen may refer to things that are filled in during the transformation stage (all those visitors in the visitor dir), such as a Symbol's name. That would cause an NPE or assertion failure.
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 cacheKey instead. Let me know if that method gives you problems. I have a bug I've been meaning to fix to improve it.
| .map((m) => methodSignatureKey(m, node.context)) | ||
| .toSet(); | ||
|
|
||
| for (final base in node.bases) { |
There was a problem hiding this comment.
Dedupe this for loop with the for (final grandBase in base.bases) { loop below. You can refactor _copyCppMethodsFromBase so you only need one .bases loop.
2 participants
No description provided.