Skip to content

Rework CAtom_setstate #234

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.

Sign up for GitHub

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

Merged
MatthieuDartiailh merged 2 commits into from
Jan 29, 2025
Merged

Rework CAtom_setstate #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab87b90
Rework CAtom_setstate
frmdstryr Jan 20, 2025
cf0d164
Use PyMapping_HasKeyWithError on 3.13+
frmdstryr Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 22 additions & 15 deletions atom/src/catom.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,29 +445,36 @@ CAtom_getstate( CAtom* self )
}

PyObject*
CAtom_setstate( CAtom* self, PyObject* args )
CAtom_setstate( CAtom* self, PyObject* state )
{
if( PyTuple_GET_SIZE( args ) != 1 )
return cppy::type_error( "__setstate__() takes exactly one argument" );
PyObject* state = PyTuple_GET_ITEM( args, 0 );
cppy::ptr itemsptr = PyMapping_Items(state);
if ( !itemsptr )
return 0;
cppy::ptr selfptr(pyobject_cast(self), true);
if ( !PyMapping_Check(state) )
return cppy::type_error("__setstate__ requires a mapping");

// If the -f key is present freeze the object
bool frozen = PyMapping_HasKey(state, atom_flags);
#if PY_VERSION_HEX >= 0x030D0000
Copy link
Member

@MatthieuDartiailh MatthieuDartiailh Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the documentation of this function but I do not understand why we need to use it here. Since we do not have the CI failure history, can you explain @frmdstryr ?

Copy link
Contributor Author

@frmdstryr frmdstryr Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's just a warning but pytest still makes it fail: See the log here https://github.com/nucleic/atom/actions/runs/12874904293/job/35895250664 .

The HasKeyWithError version makes more sense anyways.

Copy link
Member

@MatthieuDartiailh MatthieuDartiailh Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It really shows what a bad job PyMapping_Check does. I will merge.

int frozen = PyMapping_HasKeyWithError( state, atom_flags );
if ( frozen == -1 )
return 0;
#else
int frozen = PyMapping_HasKey( state, atom_flags );
#endif
if ( frozen )
{
if ( PyMapping_DelItem(state, atom_flags) == -1 )
return 0;
}

for ( Py_ssize_t i = 0; i < PyMapping_Size(state); i++ ) {
PyObject* item = PyList_GET_ITEM(itemsptr.get(), i);
PyObject* key = PyTuple_GET_ITEM(item , 0);
PyObject* value = PyTuple_GET_ITEM(item , 1);
if ( !selfptr.setattr(key, value) )
cppy::ptr iter( PyObject_GetIter(state) );
if ( !iter )
return 0;

cppy::ptr key;
while ( ( key = PyIter_Next( iter.get() ) ) )
{
cppy::ptr value( PyObject_GetItem( state, key.get() ) );
if ( !value )
return 0;
if ( PyObject_SetAttr( pyobject_cast(self), key.get(), value.get() ) )
return 0;
}

Expand Down Expand Up @@ -501,7 +508,7 @@ CAtom_methods[] = {
"__sizeof__() -> size of object in memory, in bytes" },
{ "__getstate__", ( PyCFunction )CAtom_getstate, METH_NOARGS,
"The base implementation of the pickle getstate protocol." },
{ "__setstate__", ( PyCFunction )CAtom_setstate, METH_VARARGS,
{ "__setstate__", ( PyCFunction )CAtom_setstate, METH_O,
"The base implementation of the pickle setstate protocol." },
{ 0 } // sentinel
};
Expand Down
4 changes: 2 additions & 2 deletions tests/test_get_set_state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,9 @@ class Test(AtomBase):
t.__setstate__() # Incorrect number of args
with pytest.raises(TypeError):
t.__setstate__({}, False) # Incorrect number of args
with pytest.raises(AttributeError):
with pytest.raises(TypeError):
t.__setstate__(None) # Not a mapping (no items() method)
with pytest.raises(AttributeError):
with pytest.raises(TypeError):
t.__setstate__(["z"]) # Not a mapping (has no items() method)
with pytest.raises(AttributeError):
t.__setstate__({"z": 3}) # Invalid attribute
Loading