1783 Implement Interface And Inherit Class#2028
Conversation
src/runtime/Types/MetaType.cs
Outdated
| { | ||
| if (cb2.type.Valid && cb2.type.Value.IsInterface) | ||
| interfaces.Add(cb2.type.Value); | ||
| else baseType.Add(cb2); |
There was a problem hiding this comment.
IMHO, it is better to enforce the order to be BaseClass, IInterface1, IInterface2, etc.
I believe the current code has a bug down below because it uses base_type which is not the BaseClass if the order is different.
There was a problem hiding this comment.
I have only added this functionality for types which goes through TypeManager.CreateSubType, so it should only affect that branch.
for code flows not invoking CreateSubType, the behavior willl be exactly the same as before.
lostmsu
left a comment
There was a problem hiding this comment.
Please, rebase before posting a new iteration.
P.S. sorry for the late response
| if (classBaseIt.type.Valid && classBaseIt.type.Value.IsInterface) | ||
| interfaces.Add(classBaseIt.type.Value); | ||
| else baseTypes.Add(classBaseIt); | ||
| } |
There was a problem hiding this comment.
What if it is not a .NET class or interface? You can't simply ignore it.
There was a problem hiding this comment.
Added exceptions here in the latest version.
src/runtime/Types/MetaType.cs
Outdated
| } | ||
|
|
||
| // If __assembly__ or __namespace__ are in the class dictionary then create | ||
| // If the base class has a parameterless constructor, or |
There was a problem hiding this comment.
If the base class has a parameterless constructor
??? What is this for? If that's right, it seems breaking to me.
There was a problem hiding this comment.
I forget, but that does not actually seem to be included with these changes. It is in my fork though.
| } | ||
| } | ||
|
|
||
| var base_type = Runtime.PyTuple_GetItem(bases, 0); |
There was a problem hiding this comment.
bases here can be empty since you removed check above.
There was a problem hiding this comment.
How can bases be length 0? This should only be called when a C# type is subclassed, right? In this case, it should at least be length 1.
| // That type must itself have a managed implementation. We check | ||
| // that by making sure its metatype is the CLR metatype. | ||
| // Extract interface types and base class types. | ||
| var interfaces = new List<Type>(); |
There was a problem hiding this comment.
IMHO, repeating the same base interface multiple times should be an error.
And it might be, add a test.
src/runtime/Types/MetaType.cs
Outdated
| // Multiple inheritance is not supported, unless the other types are interfaces | ||
| if (baseTypes.Count > 1) | ||
| { | ||
| return Exceptions.RaiseTypeError("cannot use multiple inheritance with managed classes"); |
There was a problem hiding this comment.
Since you collected multiple base types, perhaps it would be useful to list them in the error message in case user did not notice which ones are classes.
There was a problem hiding this comment.
Ok, I'll include this.
src/runtime/Types/MetaType.cs
Outdated
|
|
||
| if (GetManagedObject(baseTypeIt) is ClassBase classBaseIt) | ||
| { | ||
| if (classBaseIt.type.Valid && classBaseIt.type.Value.IsInterface) |
There was a problem hiding this comment.
Invalid ClassBase instances should also raise an error regardless of anything else.
There was a problem hiding this comment.
got it, adding exceptions.
Added support for multiple inheritance when inheriting from one base class and/or multiple interfaces. Added a unit test verifying that it works with a simple class and interface. This unit test would previously have failed since multiple types are in the class super class list.
- Added exceptions during unintended use of superclasses. - Removed invalid comment. - Improved exceptions from MetaType slightly.
cf4d59a to
7acbb22
Compare
|
Thanks for doing the review. I have rebased and fixed things you mentioned. Will you mark the conversations resolved where appropriate? |
|
Thank you for the contribution! |
* 1783 Implement Interface And Inherit Class Added support for multiple inheritance when inheriting from one base class and/or multiple interfaces. Added a unit test verifying that it works with a simple class and interface. This unit test would previously have failed since multiple types are in the class super class list.
Added support for multiple inheritance when inheriting from one base class and/or multiple interfaces.
Added a unit test verifying that it works with a simple class and interface. This unit test would previously have failed since multiple types are in the class super class list.
Close #1783
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG