Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented Apr 29, 2021

@erlend-aaslanderlend-aasland changed the title Use Py_TPFLAGS_IMMUTABLETYPE to check for class assignmentsbpo-43973: Use Py_TPFLAGS_IMMUTABLETYPE to check for class assignmentsApr 29, 2021
@erlend-aasland
Copy link
ContributorAuthor

cc. @vstinner

See msg392291: Can we get rid of the huge comment preceding the if statement now? Or at least reduce it considerably.

@erlend-aasland
Copy link
ContributorAuthor

AFAICS, this does not require a news item, but I might be wrong.

@erlend-aaslanderlend-aasland marked this pull request as ready for review April 29, 2021 09:32
@shreyanavigyan
Copy link
Contributor

Doesn't this also require a change?

if (compatible_for_assignment(oldto, newto, "__class__")){
if (newto->tp_flags&Py_TPFLAGS_HEAPTYPE){
Py_INCREF(newto);
}
Py_SET_TYPE(self, newto);
if (oldto->tp_flags&Py_TPFLAGS_HEAPTYPE)
Py_DECREF(oldto);
return0;
}
else{
return-1;
}

Suggested :-

 if (compatible_for_assignment(oldto, newto, "__class__")){if (!(newto->tp_flags & Py_TPFLAGS_IMMUTABLETYPE)){Py_INCREF(newto)} Py_SET_TYPE(self, newto); if (!(oldto->tp_flags & Py_TPFLAGS_IMMUTABLETYPE)) Py_DECREF(oldto); return 0} else{return -1} 

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Apr 29, 2021

Doesn't this also require a change?

AFAICT, no. They adjust the ref. count if heap types are used:

If the old class was a heap type, decrement its ref. count. If the new class is a heap type, acquire a strong reference to it (increment the ref. count).

@vstinnervstinner merged commit b73b5fb into python:masterApr 30, 2021
@erlend-aaslanderlend-aasland deleted the class_assignments branch April 30, 2021 10:07
@erlend-aasland
Copy link
ContributorAuthor

Thanks for reviewing, @vstinner !

@vstinner
Copy link
Member

Merged, thanks for the fix.

@pitrou
Copy link
Member

The comment above the check should have been fixed to reflect the new semantics, no?

Also, is the explicit check for PyModule_Type still needed?

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Apr 30, 2021

The comment above the check should have been fixed to reflect the new semantics, no?

Yes, and I believe it can be considerably reduced.

Also, is the explicit check for PyModule_Type still needed?

I don't know. See bpo-24912.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@erlend-aasland@shreyanavigyan@vstinner@pitrou@the-knights-who-say-ni@bedevere-bot