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

Should we have HPyDict_GetItem #404

Open
fangerer opened this issue Feb 8, 2023 · 0 comments
Open

Should we have HPyDict_GetItem #404

fangerer opened this issue Feb 8, 2023 · 0 comments
Labels
API design HPy update in GraalPy This change requires a related change in GraalPy HPy update in PyPy This change requires a related change in PyPy

Comments

@fangerer
Copy link
Contributor

fangerer commented Feb 8, 2023

In February 2023's dev call, we had some discussion about if we should have HPyDict_GetItem (similar to PyDict_GetItem).

PyDict_GetItem has following semantic differences compared to the generic PyObject_GetItem:

  • It will suppress any error that occurs during dict key lookup. This includes error raised from the key object's __hash__ and __eq__ methods.
  • If the first argument is an instance of dict (uses PyDict_Check internally), then it the function will directly access the dict. This means, any overwritten __getitem__ will be ignored.
  • It returns a borrowed reference.

Arguments for having HPyDict_GetItem:

  • Usage Frequency: There is one major argument for having HPyDict_GetItem in HPy: PyDict_GetItem is used a lot. On my checkout of the top4000 packages, it is used 658 times (this includes PyDict_GetItemString).
  • Error Behavior: From a C point of view, it is very convenient to just check for NULL without clearing errors.
  • Performance: It shortcuts attribute lookup and dispatch and directly accesses the storage.

Arguments against:

  • Bad API: Everyone seems to agree that this is bad API because it is inconsistent compared to many other functions.

@hodgestar made two suggestions:

  1. If the API is used for performance, the runtime should do the necessary optimizations internally. However, since the semantic is also a bit different, I think we at least need something like HPyBuiltin_GetItem (i.e. don't do an attribute lookup but access storage directly if it is an appropriate built-in type).
  2. If the API is used because of the error behavior, we could introduce HPy_GetItem_NoError to make the behavior clear.
@fangerer fangerer added API design HPy update in PyPy This change requires a related change in PyPy HPy update in GraalPy This change requires a related change in GraalPy labels Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API design HPy update in GraalPy This change requires a related change in GraalPy HPy update in PyPy This change requires a related change in PyPy
Projects
None yet
Development

No branches or pull requests

1 participant