GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-158102: Add tests for the slice C API by serhiy-storchaka · Pull Request #158120 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Sep 24, 2026

Copy link
Copy Markdown
Member

Test PySlice_Check(), PySlice_New(), PySlice_GetIndices(), PySlice_Unpack(), PySlice_AdjustIndices() and PySlice_GetIndicesEx().

PySlice_GetIndicesEx() is tested both as a macro and as a deprecated function, including the case when __index__() resizes the sequence whose size is passed as the length: the macro evaluates it after PySlice_Unpack(), the function before the call.

  • Issue: Add C API tests for the slice C API #158102

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@serhiy-storchaka @claude
pythongh-158102: Add tests for the slice C API
8536708 
Test PySlice_Check(), PySlice_New(), PySlice_GetIndices(), PySlice_Unpack(),
PySlice_AdjustIndices() and PySlice_GetIndicesEx() -- both the macro and the
deprecated function.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@serhiy-storchaka
serhiy-storchaka requested review from a team, AA-Turner, emmatyping, erlend-aasland and itamaro as code owners September 24, 2026 16:29
@bedevere-app bedevere-app Bot mentioned this pull request Sep 24, 2026
Add C API tests for the slice C API #158102
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Sep 24, 2026
@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 bugs and security fixes skip news tests Tests in the Lib/test dir labels Sep 24, 2026
@serhiy-storchaka serhiy-storchaka mentioned this pull request Sep 24, 2026
gh-158077: Remove deprecated PySlice_GetIndicesEx() function #158089
Open
vstinner
vstinner reviewed Sep 25, 2026
View reviewed changes

@vstinner vstinner 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.

Thanks for writing an exhaustive test suite for the PySlice C API. It's more complicated than what I expected! Here is a first review.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_slice.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_slice.py Outdated
Comment on lines +8 to +9
SIZE_MAX = sys.maxsize
SIZE_MIN = -sys.maxsize - 1

@vstinner vstinner Sep 25, 2026

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.

In C, SIZE_MAX is a different constant: size_t maximum (_testcapi.SIZE_MAX). Since this file tests C functions, it's confusing.

Can you rename the constants to SSIZE_MAX and SSIZE_MIN?

The _testcapi uses different names: PY_SSIZE_T_MAX and PY_SSIZE_T_MIN.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/test/test_capi/test_slice.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_slice.py Outdated
Comment on lines +11 to +13
VALUES = [None, 0, 1, 2, 3, 5, 7, -1, -2, -3, -5, -7]
STEPS = [None, 1, 2, 3, 5, -1, -2, -3, -5]
LENGTHS = [0, 1, 2, 5, 10]

@vstinner vstinner Sep 25, 2026

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.

./python -m test test_capi.test_slice -v -m test_adjustindices takes 700 ms. I'm not sure that it's worth it to test so many combinations:

Suggested change
VALUES = [None, 0, 1, 2, 3, 5, 7, -1, -2, -3, -5, -7]
STEPS = [None, 1, 2, 3, 5, -1, -2, -3, -5]
LENGTHS = [0, 1, 2, 5, 10]
VALUES = [None, 0, 1, 3, 7, -1, -3, -7]
STEPS = [None, 1, 3, 5, -1, -3, -5]
LENGTHS = [0, 1, 3, 10]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/test/test_capi/test_slice.py Outdated
# The step is asserted to be neither zero nor less than
# -PY_SSIZE_T_MAX.
# CRASHES adjust(10, 0, 10, 0)
# CRASHES adjust(10, 0, 10, SIZE_MIN)

@vstinner vstinner Sep 25, 2026

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.

PySlice_Unpack() replaces step=PY_SSIZE_T_MIN with step=PY_SSIZE_T_MAX. Would it be acceptable do the same here? Well, if we change the behavior, I would prefer to do it in a separated PR. It's better to test the current behavior without changing code first, as you do :-)

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka serhiy-storchaka Sep 25, 2026

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.

Normally, PySlice_AdjustIndices() is always called after PySlice_Unpack(), which guarantees that step is never 0 or less than -SSIZE_MAX. An additional check here would be redundant.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_slice.py

# The step is asserted to be neither zero nor less than
# -PY_SSIZE_T_MAX.
# CRASHES adjust(10, 0, 10, 0)

@vstinner vstinner Sep 25, 2026

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's unfortunate that the function cannot fail. Can you update PySlice_AdjustIndices() document to mention that step must not be equal to zero and must not be equal to PY_SSIZE_T_MIN?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@serhiy-storchaka serhiy-storchaka Sep 25, 2026 •
edited
Loading

Uh oh!

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

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'll update the docs if they already do not mentioned this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_slice.py
self.assertEqual(adjust(10, SIZE_MIN, SIZE_MAX, 1), (10, 0, 10))
self.assertEqual(adjust(10, SIZE_MAX, SIZE_MIN, -1), (10, 9, -1))
self.assertEqual(adjust(0, 1, 7, 1), (0, 0, 0))
self.assertEqual(adjust(0, 7, 1, -1), (0, -1, -1))

@vstinner vstinner Sep 25, 2026

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.

Can you add tests on negative length?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/_testlimitedcapi/slice.c Outdated

// Need limited C API 3.6.1 for PySlice_Unpack() and PySlice_AdjustIndices()
// and for PySlice_GetIndicesEx() implemented as a macro.
#if !defined(Py_GIL_DISABLED) && !defined(Py_LIMITED_API)

@vstinner vstinner Sep 25, 2026

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.

The limited C API can now be tested on Free Threading. You can also define Py_LIMITED_API if Py_GIL_DISABLED is defined, Py_TARGET_ABI3T is set to Py_LIMITED_API in this case by Include/pyabi.h.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_testlimitedcapi/slice.c Outdated
/* Test PySlice_GetIndicesEx() implemented as a macro using PySlice_Unpack()
* and PySlice_AdjustIndices(). */
static PyObject *
slice_getindicesex(PyObject *Py_UNUSED(module), PyObject *args)

@vstinner vstinner Sep 25, 2026

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.

I suggest to rename this function to "slice_getindicesex_macro()", or rename "slice_getindicesex_deprecated()" to "slice_getindicesex_func()". Same remark for the test case class name.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_slice.py Outdated
self.assertEqual(getindicesex(slice(7, 1, -2**1000), 10),
(7, 1, -SIZE_MAX, 1))
self.assertEqual(getindicesex(slice(7, 1, SIZE_MIN), 10),
(7, 1, -SIZE_MAX, 1))

@vstinner vstinner Sep 25, 2026

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.

Sadly, PySlice_GetIndicesEx() also accepts negative length. You may also add tests on negative length.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
serhiy-storchaka and others added 3 commits September 25, 2026 12:37
@serhiy-storchaka @vstinner
Apply batched suggestions from code review
1916314 
Co-authored-by: Victor Stinner <[email protected]>
@serhiy-storchaka
Document the step precondition of PySlice_AdjustIndices()
731ae1f 
Also remove trailing whitespace added by a suggestion.
@serhiy-storchaka
Address code review
764aae9 
Rename SIZE_MAX and SIZE_MIN to SSIZE_MAX and SSIZE_MIN, and the
PySlice_GetIndicesEx() wrappers to slice_getindicesex_macro() and
slice_getindicesex_func().  Test the limited C API also on free threading.
Reduce the number of tested combinations.  Add tests for zero and negative
length, and document that the length must not be negative.
@serhiy-storchaka
serhiy-storchaka requested a review from ZeroIntensity as a code owner September 25, 2026 10:05
vstinner
vstinner approved these changes Sep 25, 2026
View reviewed changes

@vstinner vstinner 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. Thanks for the update. Tests should now cover all code paths.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Sep 25, 2026
@serhiy-storchaka
serhiy-storchaka enabled auto-merge (squash) September 25, 2026 11:02
@serhiy-storchaka
serhiy-storchaka merged commit 8c056c3 into python:main Sep 25, 2026
103 of 106 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented Sep 25, 2026

Copy link
Copy Markdown

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 25, 2026
@miss-islington-app

miss-islington-app Bot commented Sep 25, 2026

Copy link
Copy Markdown

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.15 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8c056c33aacea5768f5e2010401f2b98ecd7a305 3.15
moritajavier239-rgb 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.
@miss-islington-app miss-islington-app Bot assigned serhiy-storchaka Sep 25, 2026
@miss-islington-app

miss-islington-app Bot commented Sep 25, 2026

Copy link
Copy Markdown

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8c056c33aacea5768f5e2010401f2b98ecd7a305 3.14
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented Sep 25, 2026

Copy link
Copy Markdown

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8c056c33aacea5768f5e2010401f2b98ecd7a305 3.13
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Sep 27, 2026

Copy link
Copy Markdown

GH-158291 is a backport of this pull request to the 3.15 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka deleted the slice-capi-tests branch September 27, 2026 17:34
@bedevere-app bedevere-app Bot removed the needs backport to 3.15 bugs and security fixes label Sep 27, 2026
@bedevere-app

bedevere-app Bot commented Sep 27, 2026

Copy link
Copy Markdown

GH-158292 is a backport of this pull request to the 3.14 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Sep 27, 2026
@bedevere-app

bedevere-app Bot commented Sep 27, 2026

Copy link
Copy Markdown

GH-158293 is a backport of this pull request to the 3.13 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Sep 27, 2026
serhiy-storchaka added a commit that referenced this pull request Sep 29, 2026
@serhiy-storchaka @claude @vstinner
[3.13] gh-158102: Add tests for the slice C API (GH-158120) (GH-158293)
69368eb 
Test PySlice_Check(), PySlice_New(), PySlice_GetIndices(), PySlice_Unpack(),
PySlice_AdjustIndices() and PySlice_GetIndicesEx() -- both the macro and the
deprecated function.

Document the length and step precondition of PySlice_AdjustIndices()
(cherry picked from commit 8c056c3)

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Sep 29, 2026
@serhiy-storchaka @claude @vstinner
[3.14] gh-158102: Add tests for the slice C API (GH-158120) (GH-158292)
fe0482a 
Test PySlice_Check(), PySlice_New(), PySlice_GetIndices(), PySlice_Unpack(),
PySlice_AdjustIndices() and PySlice_GetIndicesEx() -- both the macro and the
deprecated function.

Document the length and step precondition of PySlice_AdjustIndices()
(cherry picked from commit 8c056c3)

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Oct 9, 2026
@serhiy-storchaka @claude @vstinner
[3.15] gh-158102: Add tests for the slice C API (GH-158120) (GH-158291)
29abf59 
Test PySlice_Check(), PySlice_New(), PySlice_GetIndices(), PySlice_Unpack(),
PySlice_AdjustIndices() and PySlice_GetIndicesEx() -- both the macro and the
deprecated function.

Document the length and step precondition of PySlice_AdjustIndices()
(cherry picked from commit 8c056c3)

Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner
@emmatyping emmatyping Awaiting requested review from emmatyping emmatyping is a code owner
@itamaro itamaro Awaiting requested review from itamaro itamaro is a code owner
@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity ZeroIntensity is a code owner

Uh oh!

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

Assignees

@serhiy-storchaka serhiy-storchaka

Labels

skip news tests Tests in the Lib/test dir

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.

2 participants

@serhiy-storchaka @vstinner
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.