Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-110525: Add tests for internalset CAPI#110630

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
serhiy-storchaka merged 4 commits intopython:mainfromsobolevn:issue-110525-internal
Oct 10, 2023

Conversation

@sobolevn
Copy link
Member

@sobolevnsobolevn commentedOct 10, 2023•
edited by bedevere-appbot
Loading

NULLABLE(set);

rc=_PySet_NextEntry(set,&pos,&item,&hash);
if (rc==1) {

Choose a reason for hiding this comment

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

What if it returns 2 or -2?

Suggested change
if (rc==1) {
if (rc!=0&&rc!=-1) {

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sorry, I don't understand this one. Right now it is defined as:

int
_PySet_NextEntry(PyObject*set,Py_ssize_t*pos,PyObject**key,Py_hash_t*hash)
{
setentry*entry;
if (!PyAnySet_Check(set)) {
PyErr_BadInternalCall();
return-1;
}
if (set_next((PySetObject*)set,pos,&entry)==0)
return0;
*key=entry->key;
*hash=entry->hash;
return1;
}

It cannot return anything except[-1, 0, 1].
Do you mean that it can return something other than[-1, 0, 1] in the future?
I think that our test case must catch this change and be adapted if needed.

This function is not documented currently.

Copy link
Member

@serhiy-storchakaserhiy-storchakaOct 10, 2023•
edited
Loading

Choose a reason for hiding this comment

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

The purpose of the test is to verify our assumptions (that it cannot return anything except [-1, 0, 1]). Otherwise there would not be need of tests.

In future a new return can be added in the code, or refactoring can lead to returning non-initialized variable in rare case. The wrapper will successfully return None.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Got it, I used an assertion, which is more readable in my opinion 👍

@miss-islington
Copy link
Contributor

Thanks@sobolevn for the PR, and@serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry,@sobolevn and@serhiy-storchaka, I could not cleanly backport this to3.12 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 9cfb4e0d1ebf2900c19ee07697818c621f46cc3d 3.12

@sobolevn
Copy link
MemberAuthor

Working on a backport

serhiy-storchaka reacted with thumbs up emoji

@sobolevn
Copy link
MemberAuthor

@serhiy-storchaka looks like#108787 never got backported, so we don't have the same file structure on 3.12

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

Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sobolevn@miss-islington@serhiy-storchaka
[8]ページ先頭
©2009-2025 Movatter.jp