Title: Remove functions that do nothing in _Py_InitializeCore()
Type: enhancement Stage: resolved
Components: Versions:
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, ncoghlan, thomas.nyberg, vstinner
Priority: normal Keywords: patch

Created on 2018-03-01 16:57 by thomas.nyberg, last changed 2018-03-04 06:06 by benjamin.peterson. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 5953 closed thomas.nyberg, 2018-03-01 17:05
PR 5965 merged thomas.nyberg, 2018-03-03 15:59
Messages (6)
msg313100 - (view) Author: Thomas Nyberg (thomas.nyberg) * Date: 2018-03-01 16:57
The `_PyFrame_Init()` and `PyByteArray_Init()` functions are called in these two locations in the `_Py_InitializeCore()` function:

But their function definitions appear to do nothing:

I can understand leaving the functions in the source for backwards-compatibility, but why are they still being called in `_Py_InitializeCore()`? Seems like it just adds noise for those new to the cpython internals (I certainly found it confusing myself).

Ned Batchelder recommended possibly making a change:
msg313130 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-03-02 11:22
(Bringing my response from core-mentorship over to the main tracker)

These APIs are exposed to embedding applications via the pylifecycle header:

While we technically *could* deprecate & remove them (since we're not currently using them for anything), the current cost/benefit assessment is that it isn't worth the API churn (even for the underscore prefixed _PyFrame_Init API) when it's relatively cheap to keep them around as no-ops.

Given that they exist in the code base in order to continue exporting the symbols though, future maintainers are entitled to expect that we'll keep calling them in the appropriate places, such that if anyone *does* add code to them in the future, that code will "just work".
msg313139 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-03-02 17:11
Future maintainers of what exactly can expect these functions to be called? CPython? If we need to do initialization of frameobject.c later, we can simply add _PyFrame_Init back. We certainly don't support ELF-interposition of _PyFrame_Init as an API.

Historically, we've been sloppy about putting private and public APIs in the same header files. But the leading underscore still means private; such APIs serve at our pleasure.
msg313181 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-03-03 06:56
Yeah, dropping _PyFrame_Init/Fini for 3.8+ would make sense.

It's PyByteArray_Init/Fini that probably aren't worth the hassle of removing, since the lack of a leading underscore means they'd need to go through a deprecation cycle.
msg313187 - (view) Author: Thomas Nyberg (thomas.nyberg) * Date: 2018-03-03 15:59
ince I originated this issue and I think I understand the concerns, I figured I could speed up the back and forth in this thread by opening this new PR which removes the `_PyFrame_Init()` function. I couldn't find any `_PyFrame_Fini()` function in the source so maybe you two are confused and there wasn't a corresponding Fini function? (I presume the more likely scenario is that I am the one who is confused so feel free to correct that confusion in that case. :) )
msg313201 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-03-04 06:06
New changeset 7023644e0c310a3006c984318c2c111c468735b4 by Benjamin Peterson (Thomas Nyberg) in branch 'master':
closes bpo-32980 Remove _PyFrame_Init (GH-5965)
Date User Action Args
2018-03-04 06:06:03benjamin.petersonsetstatus: open -> closed
resolution: fixed
messages: + msg313201

stage: patch review -> resolved
2018-03-03 15:59:59thomas.nybergsetmessages: + msg313187
2018-03-03 15:59:11thomas.nybergsetpull_requests: + pull_request5732
2018-03-03 06:56:51ncoghlansetstatus: closed -> open
resolution: not a bug -> (no value)
messages: + msg313181

stage: resolved -> patch review
2018-03-02 17:11:55benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg313139
2018-03-02 11:22:40ncoghlansetstatus: open -> closed

nosy: + ncoghlan
messages: + msg313130

resolution: not a bug
stage: patch review -> resolved
2018-03-01 21:41:32brett.cannonsetnosy: + vstinner
2018-03-01 17:05:32thomas.nybergsetkeywords: + patch
stage: patch review
pull_requests: + pull_request5719
2018-03-01 16:57:13thomas.nybergcreate