◐ Shell
reader mode source ↗
Skip to content

gh-99845: Change _PyDict_KeysSize() return type to size_t#99848

Merged
vstinner merged 2 commits into
python:mainfrom
vstinner:dict_sizeof
Nov 29, 2022
Merged

gh-99845: Change _PyDict_KeysSize() return type to size_t#99848
vstinner merged 2 commits into
python:mainfrom
vstinner:dict_sizeof

Conversation

@vstinner

@vstinner vstinner commented Nov 28, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member
  • Change _PyDict_KeysSize() and shared_keys_usable_size() return type from signed (Py_ssize_t) to unsigned (size_t) type.
  • new_values() argument type is now unsigned (size_t).
  • init_inline_values() now uses size_t rather than int for the 'i' iterator variable.
  • type.sizeof() implementation now uses unsigned (size_t) type.

@bedevere-bot bedevere-bot mentioned this pull request Nov 28, 2022
Closed
@vstinner
gh-99845: Change _PyDict_KeysSize() return type to size_t …
b8953e1 
* Change _PyDict_KeysSize() and shared_keys_usable_size() return type
  from signed (Py_ssize_t) to unsigned (size_t) type.
* new_values() argument type is now unsigned (size_t).
* init_inline_values() now uses size_t rather than int for the 'i'
  iterator variable.
* type.__sizeof__() implementation now uses unsigned (size_t) type.
@vstinner

vstinner commented Nov 28, 2022

Copy link
Copy Markdown
Member Author

cc @methane

@vstinner

vstinner commented Nov 28, 2022

Copy link
Copy Markdown
Member Author

I would prefer to change _PyDict_SizeOf() return type to size_t, but it's not required to fix issue #99845, and it can impact the 3rd party frozendict project which uses it.

@vstinner

vstinner commented Nov 28, 2022

Copy link
Copy Markdown
Member Author

I would prefer to change _PyDict_SizeOf() return type to size_t, but it's not required to fix issue #99845, and it can impact the 3rd party frozendict project which uses it.

Another more radical change is to move the private function to internal C API, and frozendict should call dict.__sizeof__() method instead.

methane
methane approved these changes Nov 29, 2022
View reviewed changes

@methane methane 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.

Hide comment

The patch looks good to me.

Is Py_ssize_t recommended for container size and size_t recommended for memory size for now?

@vstinner vstinner merged commit 4246fe9 into python:main Nov 29, 2022
@vstinner vstinner deleted the dict_sizeof branch November 29, 2022 11:12
@vstinner

vstinner commented Nov 29, 2022

Copy link
Copy Markdown
Member Author

I was confused by assert(size > 0); is new_values(). I read it as assert(size >= 0);, to reject negative values. So I replaced it with: assert(size >= 1); which is less confusing for me :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane approved these changes

@markshannon markshannon markshannon is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vstinner @methane @bedevere-bot