Skip to content

bpo-43973: Use Py_TPFLAGS_IMMUTABLETYPE to check for class assignments - #25714

Merged
vstinner merged 1 commit into
python:masterfrom
erlend-aasland:class_assignments
Apr 30, 2021
Merged

bpo-43973: Use Py_TPFLAGS_IMMUTABLETYPE to check for class assignments#25714
vstinner merged 1 commit into
python:masterfrom
erlend-aasland:class_assignments

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Apr 29, 2021

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

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

Copy link
Copy Markdown
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

erlend-aasland commented Apr 29, 2021

Copy link
Copy Markdown
ContributorAuthor

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).

Comment threadObjects/typeobject.c Outdated
@vstinner
vstinner merged commit b73b5fb into python:masterApr 30, 2021
@erlend-aasland
erlend-aasland deleted the class_assignments branch April 30, 2021 10:07
@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

Thanks for reviewing, @vstinner !

@vstinner

Copy link
Copy Markdown
Member

Merged, thanks for the fix.

@pitrou

Copy link
Copy Markdown
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

erlend-aasland commented Apr 30, 2021

Copy link
Copy Markdown
ContributorAuthor

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