Skip to content
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

MAINT: Move offset calculation code to PyCell #1061

Closed
wants to merge 1 commit into from

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Jul 21, 2020

Follow up of #1058. Make it cleaner and correct (current code has a bug in that it assumes weakref, dict order).

@sebpuetz
Copy link
Contributor

I had just proposed something similar over at #1060, returning the offset from the start of the struct instead of the end eliminates some additional calculations, too.

#[repr(C)]
pub struct PyCell<T: PyClass> {
    inner: PyCellInner<T>,
    thread_checker: T::ThreadChecker,
    // DO NOT CHANGE THE ORDER OF THESE FIELDS WITHOUT CHANGING PyCell::dict_offset()
    // AND PyCell::weakref_offset()
    dict: T::Dict,
    weakref: T::WeakRef,
}

impl<T: PyClass> PyCell<T> {
    /// Get the offset of the dictionary from the start of the struct in bytes.
    pub(crate) fn dict_offset() -> usize {
        mem::size_of::<Self>() - mem::size_of::<T::Dict>() - mem::size_of::<T::WeakRef>()
    }

    /// Get the offset of the weakref list from the start of the struct in bytes.
    pub(crate) fn weakref_offset() -> usize {
        mem::size_of::<Self>() - mem::size_of::<T::WeakRef>()
    }
}

and

    // __dict__ support
    if !T::Dict::IS_DUMMY {
        type_object.tp_dictoffset = PyCell::<T>::dict_offset() as ffi::Py_ssize_t;
    }

    // weakref support
    if !T::WeakRef::IS_DUMMY {
        type_object.tp_weaklistoffset = PyCell::<T>::weakref_offset() as ffi::Py_ssize_t;
    }

@kngwyu
Copy link
Member Author

kngwyu commented Jul 21, 2020

Oops, I'm sorry I overlooked that.

returning the offset from the start of the struct instead of the end eliminates some additional calculations, too.

The offset should be zero if the type doesn't have dict or weakref slots, so I think rerunning Option is more convenient in that it doesn't require if offset != type_object.tp_basicsize or so.

@sebpuetz
Copy link
Contributor

As far as I can tell, my proposed solution doesn't need that if condition either? offset is eliminated from initialize_type_object entirely in that. I'll amend the commit over at #1060, should make it easier to discuss then!

@kngwyu
Copy link
Member Author

kngwyu commented Jul 21, 2020

Closed in favor of #1060

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants