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

Make _PyDict_Pop() public #111262

Closed
scoder opened this issue Oct 24, 2023 · 6 comments
Closed

Make _PyDict_Pop() public #111262

scoder opened this issue Oct 24, 2023 · 6 comments
Labels
topic-C-API type-feature A feature request or enhancement

Comments

@scoder
Copy link
Contributor

scoder commented Oct 24, 2023

Feature or enhancement

Proposal:

It's used, it's reasonable, it's fast, it's already there. Just expose it instead of deleting it.

See #106320 and #108449

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

@scoder scoder added the type-feature A feature request or enhancement label Oct 24, 2023
@serhiy-storchaka
Copy link
Member

We should first discuss the design.

The flaws of the current _PyDict_Pop():

  • It is difficult to distinguish two cases: return value removed from the dict and returning the increfed default value. If the dict can contain arbitrary values, you need to create a special sentinel value.
  • If you do not need the default value, you still have to decref it.

Alternative designs of PyDict_Pop() that do not raise KeyError.

  1. PyObject *PyDict_Pop(PyObject *dict, PyObject *key)

    • Return NULL and set exception on error.
    • Return NULL and not set exception if the key is not found.
    • Return the removed value if the key is found.

    You have to use PyErr_Occurred() to distinguish between first two cases.

  2. int PyDict_Pop(PyObject *dict, PyObject *key, PyObject **result)

    • Return -1, set *result to NULL and set exception on error.
    • Return 0, set *result to NULL and not set exception if the key is not found.
    • Return 1, set *result to the removed value if the key is found.

    You have not to use PyErr_Occurred(). You have to check the returning code or *result and *result to the default values after the call if you need it.

  3. int PyDict_Pop(PyObject *dict, PyObject *key, PyObject **result)

    • Return -1, set *result to NULL and set exception on error.
    • Return 0, incref *result (which should be initialized with the default value) and not set exception if the key is not found.
    • Return 1, set *result to the removed value if the key is found.

    You have to always initialize *result before the call, but you can simply use *result after the call if you treat two last cases the same.

These variants do not raise KeyError. If you need KeyError, you have to raise it yourself. Or we can introduce new function which pops item and raises KeyError, or add a flag to raise KeyError. The last two variants have subvariants, how to treat result == NULL:

  • Simply drop the removed value. It is convenient if PyDict_Pop() is used as a variant of PyDict_DelItem() that tolerates absent keys. It is the most of use cases in the CPython sources.
  • Treat it as a signal to raise KeyError.

cc @rhettinger, @vstinner

@vstinner
Copy link
Member

I like to return (-1, 0, 1). But I didn't get details of the API. The definition has not default argument? What is the behavior is default is NULL and the key does not exist.

Is it possible to call it with key=NULL?

@scoder
Copy link
Contributor Author

scoder commented Oct 25, 2023

Ok, I get the pattern. I'll add _PyDict_Pop() to #111162 so that we have an API until the bike-shedding has settled.

scoder added a commit to scoder/cpython that referenced this issue Oct 25, 2023
encukou pushed a commit that referenced this issue Oct 25, 2023
* gh-106320: Re-add _PyLong_FromByteArray(), _PyLong_AsByteArray() and _PyLong_GCD() to the public header files since they are used by third-party packages and there is no efficient replacement.

See #111140
See #111139

* gh-111262: Re-add _PyDict_Pop() to have a C-API until a new public one is designed.
@vstinner
Copy link
Member

Ok, I get the pattern. I'll add _PyDict_Pop() to #111162 so that we have an API until the bike-shedding has settled.

That sounds reasonable since 300+ private functions were removed, I don't expect that we can add public replacement quickly enough.

vstinner added a commit to vstinner/cpython that referenced this issue Nov 12, 2023
Change the API of the internal _PyDict_Pop_KnownHash() function
to return an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
Change the API of the internal _PyDict_Pop_KnownHash() function
to return an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
Change the API of the internal _PyDict_Pop_KnownHash() function
to remove the default value and return an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
Change the API of the internal _PyDict_Pop_KnownHash() function
to remove the default value and return an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
Change the API of the internal _PyDict_Pop_KnownHash() function
to remove the default value and return an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
Change the API of the internal _PyDict_Pop_KnownHash() function
to remove the default value and return an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 13, 2023
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit to vstinner/cpython that referenced this issue Nov 14, 2023
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
vstinner added a commit that referenced this issue Nov 14, 2023
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
@vstinner
Copy link
Member

After a long discussion, the int PyDict_Pop(PyObject *p, PyObject *key, PyObject **result) API won: there is no default value anymore. Thanks everyone for this interesting discussion.

Thanks @scoder for raising the need for such API.

@vstinner
Copy link
Member

I wrote python/pythoncapi-compat#81 to add PyDict_Pop() and PyDict_PopString() to pythoncapi-compat: it calls _PyDict_Pop() on CPython >= 3.5.0b2, or call the dict.pop() method on older Cython versions and on PyPy.

aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
…#111162)

* pythongh-106320: Re-add _PyLong_FromByteArray(), _PyLong_AsByteArray() and _PyLong_GCD() to the public header files since they are used by third-party packages and there is no efficient replacement.

See python#111140
See python#111139

* pythongh-111262: Re-add _PyDict_Pop() to have a C-API until a new public one is designed.
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
…#111162)

* pythongh-106320: Re-add _PyLong_FromByteArray(), _PyLong_AsByteArray() and _PyLong_GCD() to the public header files since they are used by third-party packages and there is no efficient replacement.

See python#111140
See python#111139

* pythongh-111262: Re-add _PyDict_Pop() to have a C-API until a new public one is designed.
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants