Skip to content

Conversation

@colesbury
Copy link
Contributor

@colesburycolesbury commented Nov 17, 2023

This changes a number of internal usages of PyDict_SetDefault to use PyDict_SetDefaultRef.

Copy link
Member

@encukouencukou left a comment

Choose a reason for hiding this comment

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

+1 from me; IMO the chance of C-API WG having objections is low enough, let's be optimistic and merge this.

Comment on lines 986 to 990
if (res<0){
Py_DECREF(key);
returnNULL;
}
if (res==1){

Choose a reason for hiding this comment

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

Maybe merge them in single test != 0?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks - I combined the cases.

…fault`. This changes a number of internal usages of `PyDict_SetDefault` to use `PyDict_SetDefaultRef`.
@colesburycolesburyforce-pushed the PyDict_SetDefaultRef-usage branch from 3899746 to 14a00f4CompareFebruary 6, 2024 16:38
@colesburycolesbury marked this pull request as ready for review February 6, 2024 16:39
@colesbury
Copy link
ContributorAuthor

I've rebased this PR now that #112123 is merged. @serhiy-storchaka, would you please look over this?

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@colesburycolesbury merged commit ef3ceab into python:mainFeb 7, 2024
@colesburycolesbury deleted the PyDict_SetDefaultRef-usage branch February 7, 2024 18:43
@vstinner
Copy link
Member

Nice change, it makes the code more readable.

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
…fault`. (python#112211) This changes a number of internal usages of `PyDict_SetDefault` to use `PyDict_SetDefaultRef`. Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Sign up for freeto 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.

5 participants

@colesbury@vstinner@encukou@serhiy-storchaka@erlend-aasland