GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-111262: Add PyDict_Pop() function [without default value nor KeyError] by vstinner · Pull Request #112028 · python/cpython

Conversation

vstinner commented Nov 13, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

_PyDict_Pop_KnownHash(): remove the default value and the return type becomes an int.

Issue: Make _PyDict_Pop() public #111262

📚 Documentation preview 📚: https://cpython-previews--112028.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner requested review from a team, encukou, kumaraditya303, markshannon, methane and rhettinger as code owners

November 13, 2023 11:53

bedevere-app Bot added the awaiting core review label

Nov 13, 2023

bedevere-app Bot mentioned this pull request

Nov 13, 2023

Make _PyDict_Pop() public #111262

Closed

vstinner added the topic-C-API label

Nov 13, 2023

vstinner mentioned this pull request

Nov 13, 2023

gh-111262: Add PyDict_Pop() function [with default value] #111939

Closed

vstinner commented Nov 13, 2023

Copy link

Copy Markdown

Member Author

This PR is based on PR gh-111939 (which was based on PR gh-111263). This API is different:

Remove default_value parameter
result argument can be NULL: it's common that the removed value is not used in the caller, see the PR
Don't raise KeyError if the key is not present

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner changed the title ~~gh-111262: Add PyDict_Pop() function~~ gh-111262: Add PyDict_Pop() function [without default value nor KeyError]

Nov 13, 2023

vstinner commented Nov 13, 2023

Copy link

Copy Markdown

Member Author

Simplified example where PyDict_Pop() is used to remove a dict key without caring of the removed value:

static int
sys_set_object(PyInterpreterState *interp, PyObject *key, PyObject *v)
{
    PyObject *sd = interp->sysdict;
    if (v == NULL) {
        if (PyDict_Pop(sd, key, NULL) < 0) {
            return -1;
        }
        // no need to care about KeyError or Py_DECREF()
        return 0;
    }
    else {
        return PyDict_SetItem(sd, key, v);
    }
}

Example which uses the removed value:

PyObject *ob;
if (PyDict_Pop(kwargs, key, &ob) < 0) {
    goto error;
}
if (ob != NULL) {
    result->ob_item[i] = ob;
}
else {
    // no need to care about KeyError or Py_DECREF()
    result->ob_item[i] = Py_NewRef(self->ob_item[i]);
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Nov 13, 2023

Copy link

Copy Markdown

Member Author

The function is not added to the limited C API: @encukou asks to wait until Python will have a C API Working Group.

zooba reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

Nov 13, 2023

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This PR includes yet one change. It allows to pass NULL as a result address. It complicates the implementation, but makes the common use case simpler.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/structseq.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/structseq.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/sysmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/dictobject.c



		PyObject *
		_PyDict_Pop(PyObject dict, PyObject key, PyObject *default_value)

serhiy-storchaka Nov 13, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why not remove it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Nov 13, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I prefer to remove it in a separated PR: see #112026

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/socketmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_testcapi/dict.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_testcapi/dict.c

		return NULL;
		}
		if (result == NULL) {
		result = Py_NewRef(Py_None);

serhiy-storchaka Nov 13, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	result = Py_NewRef(Py_None);
	return PyLong_FromLong(res);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Nov 13, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I prefer to return (0, None) to make the tests written in Python closer to what the C API returns. In test_capi.test_dict, you can see that as (0, NULL).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Nov 13, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It can be confused with actual None. For example dict.get() return an actual none, but several corresponding C API functions return NULL.

In other tests I made them returning AttributeError or KeyError, as it is less chance to confuse with real value, but I think that it would be better to use a special singleton _testcapi.MISSING in future.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_testcapi/dict.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_capi/test_dict.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_capi/test_dict.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Nov 13, 2023

Copy link

Copy Markdown

Member Author

As requested, I added PyDict_PopString() function.

I added tests on PyDict_PopString() and PyDict_Pop(dict, key, NULL).

I also addresssed @serhiy-storchaka's review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

Nov 13, 2023

View reviewed changes

Comment thread

Lib/test/test_capi/test_dict.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_capi/test_dict.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_capi/test_dict.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_testcapi/dict.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_testcapi/dict.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner mentioned this pull request

Nov 13, 2023

Return value conventions capi-workgroup/api-evolution#13

Open

vstinner force-pushed the dict_pop2 branch from fa618e6 to 7c84afb Compare

November 13, 2023 23:04

vstinner commented Nov 13, 2023

Copy link

Copy Markdown

Member Author

@serhiy-storchaka: I added more tests.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka approved these changes

Nov 14, 2023

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

What interface do you prefer, Victor? This or #111939? I am ready to make a review of #111939 if you add corresponding *String variant and tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Nov 14, 2023

scoder approved these changes

Nov 14, 2023

View reviewed changes

scoder left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM overall, just wording fixes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/whatsnew/3.13.rst

Comment on lines +1171 to +1181

		* Add :c:func:`PyDict_Pop` and :c:func:`PyDict_PopString` functions: remove a
		key from a dictionary and optionally return the removed value. This is the
		similar to :meth:`dict.pop`, but without the default value and do not raise
		:exc:`KeyError` if the key missing.

scoder Nov 14, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's usually clearer to write "not raising an exception" (at all) than naming an explicit exception and leaving it open whether other exceptions might be raised in the described case.

Suggested change

	* Add :c:func:`PyDict_Pop` and :c:func:`PyDict_PopString` functions: remove a
	key from a dictionary and optionally return the removed value. This is the
	similar to :meth:`dict.pop`, but without the default value and do not raise
	:exc:`KeyError` if the key missing.
	* Add :c:func:`PyDict_Pop` and :c:func:`PyDict_PopString` functions: remove a
	key from a dictionary and optionally return the removed value. This is
	similar to :meth:`dict.pop`, but without the default value and not raising
	an exception if the key missing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Nov 14, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks. I applied your suggestion to the 3 places where I wrote that doc, but I kept KeyError. IMO it's useful to be explicit about KeyError to understand what we are talking about.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Misc/NEWS.d/next/C API/2023-11-10-10-21-38.gh-issue-111262.2utB5m.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Nov 14, 2023

Copy link

Copy Markdown

Member Author

@serhiy-storchaka:

What interface do you prefer, Victor?

At the beginning, I had no idea. It was very blurry. What helped me to make my own opinion was to actually use the different proposed API and look at the updated code. I really like this API without the default value: I'm convinced by these examples.

In short, I prefer this PR.

serhiy-storchaka reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner and others added 5 commits

November 14, 2023 13:19


          pythongh-111262: Add PyDict_Pop() function

_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-Authored-By: Stefan Behnel <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>


          Remove PyDict_Pop() from stable ABI

ae73490


          sys_set_object() ignores removed value

f65c10f


          Add PyDict_PopString() function

9da06c8


          Make tests more explicit that NULL is returned if key is missing

343f75a

vstinner added 4 commits

November 14, 2023 13:19


          Add tests

2170fc0


          Enhance tests

c3b8bd2


          Add more tests

a286604


          Update doc

229f524

vstinner force-pushed the dict_pop2 branch from e988e07 to 229f524 Compare

November 14, 2023 12:19

vstinner enabled auto-merge (squash)

November 14, 2023 12:19

vstinner merged commit 4f04172 into python:main

Nov 14, 2023

vstinner deleted the dict_pop2 branch

November 14, 2023 12:51

bedevere-app Bot removed the awaiting merge label

Nov 14, 2023

vstinner mentioned this pull request

Nov 14, 2023

gh-111262: Make PyDict_Pop() public #111263

Closed

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-111262: Add PyDict_Pop() function (python#112028)

024183c

_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-111262: Add PyDict_Pop() function (python#112028)

99d4048

_PyDict_Pop_KnownHash(): remove the default value and the return type
becomes an int.

Co-authored-by: Stefan Behnel <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>

Reviewers

scoder

scoder approved these changes

serhiy-storchaka

serhiy-storchaka approved these changes

rhettinger

Awaiting requested review from rhettinger

rhettinger is a code owner

encukou

Awaiting requested review from encukou

kumaraditya303

Awaiting requested review from kumaraditya303

kumaraditya303 is a code owner

methane

Awaiting requested review from methane

methane is a code owner

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-C-API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

vstinner commented Nov 13, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Nov 13, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Nov 14, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vstinner commented Nov 13, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.