| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
We should include documentation on the type object page too.
Sorry, something went wrong.
|
@ZeroIntensity Thanks for review! Fixed (I hope :) |
Sorry, something went wrong.
There was a problem hiding this comment.
I think that if this is internal-only, it would be better to completely drop it from public headers rather than document it as such.
Sorry, something went wrong.
(Don't do this yet, by the way. Let's come to a consensus in the issue.) |
Sorry, something went wrong.
|
As no comments from @iritkatriel or @markshannon I propose to make it internal as @ZeroIntensity suggested and rename it to _Py_TPFLAGS_PREHEADER. @encukou WDYT? |
Sorry, something went wrong.
|
Per PEP-387, removing (or renaming) it probablyneeds a deprecation cycle. Or is there evidence that no existing code uses it? |
Sorry, something went wrong.
|
Since the tp_flags flags must be public, it seemed sensible to put Py_TPFLAGS_PREHEADER in the same header. It might be better to rename it to Py_TPFLAGS_VM_MANAGED_FIELDS to focus on the semantics, not the layout, and keep it public? |
Sorry, something went wrong.
https://grep.app/search?q=Py_TPFLAGS_PREHEADER shows close to nothing of usage. Also, I'm downloading top-pypi packages to checks usage (as described here https://hugovk.dev/blog/2022/how-to-search-5000-python-projects/), will report as it finished. |
Sorry, something went wrong.
@markshannon WDYT about wordings in this PR? Can you suggest some improvements? |
Sorry, something went wrong.
Searched over 13k+ packages: ➜ .\python.exe ../misc/cpython/search_pypi_top.py -q ..\top-pypi\output\ "Py_TPFLAGS_PREHEADER" ..\top-pypi\output\recordclass-0.23.1.tar.gz: recordclass-0.23.1/lib/recordclass/_dataobject.c: tp->tp_flags &= ~Py_TPFLAGS_PREHEADER; Time: 0:01:10.586519 Found 1 matching lines in 1 projects ➜ ( Get-ChildItem ..\top-pypi\output\ | Measure-Object ).Count; 13961 |
Sorry, something went wrong.
If it's public, it's best not to rename it. |
Sorry, something went wrong.
Co-authored-by: Petr Viktorin <encukou@gmail.com>
I think it's probably safe to rename it, at least for 3.14. Based on a code search, we're the only ones who use it (all the non-CPython mentions are people copying our headers). |
Sorry, something went wrong.
Only recordclass uses it - #135861 (comment) |
Sorry, something went wrong.
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
|
@ZeroIntensity Please take a look. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, this mostly looks good.
Sorry, something went wrong.
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
There was a problem hiding this comment.
LGTM.
I'm not a huge fan of documenting things that should be private, but I think it's too late to move this to internal headers or rename it. We should consider deprecating this in 3.15.
@encukou Would you like to finish up your review?
Sorry, something went wrong.
|
It's hard to deprecate a macro, so adding warnings in docs look best. |
Sorry, something went wrong.
|
Thanks everyone! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
I see following usages of Py_TPFLAGS_PREHEADER:
I checked all linked PR from #95245 and can't find any explanation. I also checked https://github.com/python/cpython/blob/main/Objects/object_layout.md.
@encukou Please take a look.