Repository navigation
Conversation
| @@ -849,7 +861,23 @@ impl<'a, T: PyObjectPayload> From<&'a Py<T>> for &'a PyObject { | |||
| impl Drop for PyObjectRef { | |||
| #[inline] | |||
| fn drop(&mut self) { | |||
There was a problem hiding this comment.
Because PyObjectRef::drop is very small, I'd like to suggest to add a whole debug drop , rather than adding a lot bigger debug features inside of the functions.
#[cfg(debug_assertions)]
fn drop(&mut self) {
// debug drop here
}
#[cfg(not(debug_assertions))
#[inline]
fn drop(&mut self) {
// production drop here
}There was a problem hiding this comment.
Because
PyObjectRef::dropis very small, I'd like to suggest to add a whole debug drop , rather than adding a lot bigger debug features inside of the functions.#[cfg(debug_assertions)] fn drop(&mut self) { // debug drop here } #[cfg(not(debug_assertions)) #[inline] fn drop(&mut self) { // production drop here }
Doing the refactor right now, after I done refactor, should I amend&force push or create a new commit?
There was a problem hiding this comment.
Because
PyObjectRef::dropis very small, I'd like to suggest to add a whole debug drop , rather than adding a lot bigger debug features inside of the functions.#[cfg(debug_assertions)] fn drop(&mut self) { // debug drop here } #[cfg(not(debug_assertions)) #[inline] fn drop(&mut self) { // production drop here }
Also I refactor GC PR(https://github.com/discord9/RustPython/blob/7a3ba7884aec20f42f1bddf290a0e93beb13e87e/vm/src/object/core.rs#L1099) on top of this drop check, which, you can see, make drop() a much larger function, should it be spilted the same way?
impl Drop for PyObjectRef {
#[inline]
fn drop(&mut self) {
#[cfg(debug_assertions)]
if *self.0.is_drop.lock() {
error!(
"Double drop on PyObjectRef with typeid={:?}(Type={:?})",
self.0.typeid,
ID2TYPE
.lock()
.expect("can't read ID2TYPE")
.get(&self.0.typeid),
);
return;
}
#[cfg(feature = "gc")]
{
let stat = self.decrement();
match stat {
GcStatus::ShouldDrop => unsafe {
*self.0.is_drop.lock() = true;
PyObject::drop_slow(self.ptr);
},
GcStatus::ShouldDropOnly | GcStatus::BufferedDrop => unsafe {
*self.0.is_drop.lock() = true;
PyObject::drop_only(self.ptr);
},
GcStatus::ShouldKeep | GcStatus::DoNothing => (),
}
}
#[cfg(not(feature = "gc"))]
if self.0.ref_count.dec() {
#[cfg(debug_assertions)]
{
*self.0.is_drop.lock() = true;
}
unsafe { PyObject::drop_slow(self.ptr) }
}
}
}There was a problem hiding this comment.
Because they don't share any logic, having different drop function for gc feature looks reasonable.
|
I would suggest we first fix #4238 before merge this PR, or the error message can be rather annoying for every cargo run pop up dozens of those error now. |
|
Gonna to integrate drop check with gc, so closing this for now |
Add a double drop check field for PyInner, which:
is_dropfield back to false, also might cause seg fault if others write even more exotic data in it after drop[ERROR]message, but doesn't panicTypeId's SipHash impl doesn't collide with so few types)