FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-130080: move _Py_EnsureArrayLargeEnough to a separate header so it can be used outside of the compiler by iritkatriel · Pull Request #130930 · python/cpython · GitHub

/ cpython Public

gh-130080: move _Py_EnsureArrayLargeEnough to a separate header so it can be used outside of the compiler - #130930

Merged
iritkatriel merged 9 commits into
python:mainfrom
iritkatriel:array
Mar 13, 2025
Merged

gh-130080: move _Py_EnsureArrayLargeEnough to a separate header so it can be used outside of the compiler#130930
iritkatriel merged 9 commits into
python:mainfrom
iritkatriel:array

Conversation

iritkatriel commented Mar 6, 2025
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

This function is not specific to the compiler and I have a use case for it in another area of the codebase.

Comment thread Include/internal/pycore_c_array.h Outdated
Comment thread Include/internal/pycore_c_array.h Outdated
Comment thread Include/internal/pycore_c_array.h Outdated

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it would make sense to define a struct for this list, then pass a pointer to the struct rather than 5 parameters. Less error prone and easier to read, IMO.

Comment thread Include/internal/pycore_c_array.h Outdated
Comment thread Include/internal/pycore_c_array.h Outdated

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some nit/question

Comment thread Include/internal/pycore_c_array.h Outdated
*
* Return 0 if successful and -1 (with exception set) otherwise.
*/
int _Py_c_array_EnsureCapacity(_Py_c_array_t *c_array, int idx);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
int _Py_c_array_EnsureCapacity(_Py_c_array_t *c_array, int idx);
int
_Py_c_array_EnsureCapacity(_Py_c_array_t *c_array, int idx);

Should we assume idx is n int or maybe a uint32_t or just a size_t? any possibility of unsafe downcasting?

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Adding a struct for just the one function and no information hiding seems a bit of a waste.

What I had in mind, was a struct that could be used as a variable sized array/vector.

typedef struct _Py_c_array_t {
    void *array;
    int allocated;   /* capacity of the array */
    size_t item_size;         /* size of each element */
}_Py_c_array_t;

int _Py_CArray_Init(_Py_c_array_t *array, Py_ssize_t itemsize, int initial_capacity);
int _Py_CArray_EnsureCapacity(_Py_c_array_t *array, int idx);
void _Py_CArray_Fini(_Py_c_array_t *array);

Although using it would require changing basicblock from:

    cfg_instr *b_instr;
    int b_ialloc;

to

    _Py_c_array_t b_instr;

and changing every use of b_instr and b_ialloc, so you might not want to do that.

One other thing to consider is do you want a resizeable array or a vector (in C++ parlance)?
b_instr is conceptually a vector, we never leave gaps, and generally add at the end.
If you want to make it a vector, add a used field and a int _Py_CArray_Push(_Py_c_array_t *array, void *item); function.

Comment thread Python/codegen.c Outdated
Comment thread Python/flowgraph.c
&b->b_ialloc,
DEFAULT_BLOCK_SIZE,
sizeof(cfg_instr)));
_Py_c_array_t array = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Having to manually initialize a struct, just to pass it to a function seems clunky. The previous API was probably better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is just to make the transition now. If we were defining basic block from scratch we would have put this struct field in it. But I think now that's a transformation for another PR.

Copy link
Copy Markdown
Member Author

One other thing to consider is do you want a resizeable array or a vector (in C++ parlance)?

It's a resizable array. The labelsmap is not a vector.

markshannon self-requested a review March 12, 2025 17:07
iritkatriel merged commit 4242c2b into python:main Mar 13, 2025
plashchynski pushed a commit to plashchynski/cpython that referenced this pull request Mar 17, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL