New subclass payload layout #6319
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
557b273 to
91975e2
Compare
crates/vm/src/builtins/bool.rs
Outdated
| #[inline] | ||
| fn payload_type_id() -> std::any::TypeId { | ||
| std::any::TypeId::of::<PyInt>() | ||
| } |
There was a problem hiding this comment.
This is okay here, because PyInt and PyBool shares same payload. The only difference is type.
But implementing this makes PyPayload::downcastable to be automatically true for any kind of PyInt. It doesn't make failing tests on this patch because we don't use that pattern. But it obviously will be a sort of trap for future developing.
Another question is that PyPayload::downcastable only takes &PyObject currently. We can't compare it to actual type... because we don't have vm.ctx.
| #[pyclass(name = "bool", module = false, base = PyInt)] | ||
| pub struct PyBool; | ||
| #[repr(transparent)] | ||
| pub struct PyBool(pub PyInt); |
There was a problem hiding this comment.
Now we have to manually define the base layout to be always leading field.
The problem is Rust actually only guarantees it to be actually same layout only when #[repr(transparent)] is used.
What I didn't check yet: Can using #[repr(C)] helps? Is there any other repr options to keep the first field as same as its original layout?
e312bbf to
8db2660
Compare
ShaharNaveh
left a comment
There was a problem hiding this comment.
Looks super cool. I am a bit worried that we are not actually using it anywhere ATM so we can't validate that it's not a breaking change.
How difficult would it be to gradually transition to pattern?
afe436d to
4e2edcc
Compare
|
|
||
| #[inline] | ||
| fn validate_downcastable_from(obj: &::rustpython_vm::PyObject) -> bool { | ||
| <Self as ::rustpython_vm::class::PyClassDef>::BASICSIZE <= obj.class().slots.basicsize && obj.class().fast_issubclass(<Self as ::rustpython_vm::class::StaticType>::static_type()) |
There was a problem hiding this comment.
Maybe I found another way to do with more tricks. Let's check it with OSError
4e2edcc to
1a4a712
Compare
1a4a712 to
9f82068
Compare
9f82068 to
d2f933e
Compare
I am considering new subclass payload layout to provide
I will left open questions on the code.