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

Use _Py_EnterRecursiveCall private API in _bisectmodule.c #101903

Closed
gdementen opened this issue Feb 14, 2023 · 4 comments
Closed

Use _Py_EnterRecursiveCall private API in _bisectmodule.c #101903

gdementen opened this issue Feb 14, 2023 · 4 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@gdementen
Copy link

gdementen commented Feb 14, 2023

Feature or enhancement

I just noticed the _bisectmodule.c is the only internal module using the Py_EnterRecursiveCall and Py_LeaveRecursiveCall public API functions instead of the private ones with underscores. For both consistency and a tiny performance enhancement, I think it would make sense to use the private API instead.

Pitch

I have a PR ready for this, but since I have no idea of what I am doing this might be completely bogus 😉.

Linked PRs

@gdementen gdementen added the type-feature A feature request or enhancement label Feb 14, 2023
@OTheDev
Copy link
Contributor

OTheDev commented Feb 14, 2023

Looks like the Public API functions

cpython/Python/ceval.c

Lines 3066 to 3081 in 23751ed

/* Implement Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() as functions
for the limited API. */
#undef Py_EnterRecursiveCall
int Py_EnterRecursiveCall(const char *where)
{
return _Py_EnterRecursiveCall(where);
}
#undef Py_LeaveRecursiveCall
void Py_LeaveRecursiveCall(void)
{
_Py_LeaveRecursiveCall();
}

are just wrappers for the private static inline functions

static inline int _Py_EnterRecursiveCall(const char *where) {
PyThreadState *tstate = _PyThreadState_GET();
return _Py_EnterRecursiveCallTstate(tstate, where);
}
static inline void _Py_LeaveRecursiveCallTstate(PyThreadState *tstate) {
tstate->c_recursion_remaining++;
}
static inline void _Py_LeaveRecursiveCall(void) {
PyThreadState *tstate = _PyThreadState_GET();
_Py_LeaveRecursiveCallTstate(tstate);
}

@gdementen - I don't see the harm in submitting a PR!

@erlend-aasland
Copy link
Contributor

As noted in #101931 (comment), this requires defining _bisectmodule.c as a core module, which IMO requires a broader discussion.

We deliberately make some stdlib extension modules use the public C API. As an (unrwritten) general rule, the modules in Modules/Setup.bootstrap.in are built using the internal C API, whereas the modules in Modules/Setup.stdlib.in should preferably use the public C API.

Suggesting to close this issue as not-planned.

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Feb 21, 2023
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Feb 21, 2023

See also Eric's comments from similar API issues: #31376 (comment) and #31376 (comment)

@erlend-aasland erlend-aasland closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2023
@erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label Feb 21, 2023
@OTheDev
Copy link
Contributor

OTheDev commented Feb 21, 2023

Thanks @erlend-aasland

carljm added a commit to carljm/cpython that referenced this issue Feb 23, 2023
* main: (76 commits)
  Fix syntax error in struct doc example (python#102160)
  pythongh-99108: Import MD5 and SHA1 from HACL* (python#102089)
  pythonGH-101777: `queue.rst`: use 2 spaces after a period to be consistent. (python#102143)
  Few coverage nitpicks for the cmath module (python#102067)
  pythonGH-100982: Restrict `FOR_ITER_RANGE` to a single instruction to allow instrumentation. (pythonGH-101985)
  pythongh-102135: Update turtle docs to rename wikipedia demo to rosette (python#102137)
  pythongh-99942: python.pc on android/cygwin should link to libpython per configure.ac (pythonGH-100356)
  pythongh-95672 fix typo SkitTest to SkipTest (pythongh-102119)
  pythongh-101936: Update the default value of fp from io.StringIO to io.BytesIO (pythongh-102100)
  pythongh-102008: simplify test_except_star by using sys.exception() instead of sys.exc_info() (python#102009)
  pythongh-101903: Remove obsolete undefs for previously removed macros Py_EnterRecursiveCall and Py_LeaveRecursiveCall (python#101923)
  pythongh-100556: Improve clarity of `or` docs (python#100589)
  pythongh-101777: Make `PriorityQueue` docs slightly clearer (python#102026)
  pythongh-101965: Fix usage of Py_EnterRecursiveCall return value in _bisectmodule.c (pythonGH-101966)
  pythongh-101578: Amend exception docs (python#102057)
  pythongh-101961 fileinput.hookcompressed should not set the encoding value for the binary mode (pythongh-102068)
  pythongh-102056: Fix a few bugs in error handling of exception printing code (python#102078)
  pythongh-102011: use sys.exception() instead of sys.exc_info() in docs where possible (python#102012)
  pythongh-101566: Sync with zipp 3.14. (pythonGH-102018)
  pythonGH-99818: improve the documentation for zipfile.Path and Traversable (pythonGH-101589)
  ...
python-sidebar pushed a commit to python-sidebar/Python-Documentation-Fork-With-TOC that referenced this issue Sep 1, 2024
python-sidebar pushed a commit to python-sidebar/Python-Documentation-Fork-With-TOC that referenced this issue Sep 1, 2024
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this issue Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants